Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tutorial for (mostly Symbol based) interfacing with problems, integrators, and solutions #639

Merged
merged 13 commits into from
Apr 27, 2023

Conversation

TorkelE
Copy link
Member

@TorkelE TorkelE commented Apr 12, 2023

Add a new tutorial on how to e.g. do sol[:X1] to get solution values of X1. Also mentioned remake and how to do

@unpack X1 = rn

and use X1 instead of :X1.

Makes minor modifications in the callback and plotting description of the advanced simulation tutorial to use this notation.

@TorkelE
Copy link
Member Author

TorkelE commented Apr 12, 2023

This should be ready for merging, if checks pass.

docs/src/catalyst_applications/advanced_simulations.md Outdated Show resolved Hide resolved
end
cb = PresetTimeCallback(condition, affect!)

sol = solve(deepcopy(jprob), SSAStepper(); callback=cb)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sol = solve(deepcopy(jprob), SSAStepper(); callback=cb)
sol = solve(jprob, SSAStepper(); callback=cb)

Why are you using deepcopy? I wouldn't use this in a tutorial as it will just lead to confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though deepcopy was the way to go for complicated structures with other structures within them? Happy to be corrected though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you copying it at all though? You just defined it 11 lines above.

The only reason to deepcopy is if you are going to change something in it and want to keep the original around, but you never use it again so I'm not sure why it being mutated matters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly this is the last solve of the problem in this example, but assuming hypothetical following code this seems sensible though? Or imagine someone just rerunning that last line 2-3 times to see different solutions.

It feels like good practise to pass a copy of the problem when using mutating callbacks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly this is the last solve of the problem in this example, but assuming hypothetical following code this seems sensible though? Or imagine someone just rerunning that last line 2-3 times to see different solutions.

It feels like good practise to pass a copy of the problem when using mutating callbacks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better example here is to run and plot like 10 solutions. Then you would need to keep the original around as indeed I think you'd have an issue without using deep copies due to the mutation (i.e. the problem would be in an inconsistent state on the next simulation). Then you can use deepcopy, but I'd also add an explanation then for why it is needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But to be honest, the better approach would be to show users how to then reset things via mutation of the original problem again to have the original values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to keep the example focused, and not make a full ensemble as that is pretty much a repeat of another tutorial. Will do some more explaining of deepcopy, and link the ensemble example where I actually show why the deepcopy is important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But to be honest, the better approach would be to show users how to then reset things via mutation of the original problem again to have the original values.

How you mean, just reset the value after the simulation has finished?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so this example is proceeded by one where we explain, with examples, why deepcopy is needed (line 185 to 265). I think is is well explained, but tell me if you think there should be further changes.

TorkelE and others added 10 commits April 13, 2023 20:17
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
…g.md

Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
…g.md

Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
…g.md

Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
…g.md

Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@isaacsas
Copy link
Member

Could you fix the examples that are failing and giving errors when building the docs?

After that feel free to merge -- I can't really read through till mid-next week, and I can just tweak it if I want to update anything at that time.

@TorkelE TorkelE merged commit 1273439 into master Apr 27, 2023
@isaacsas isaacsas deleted the update_doc_interfacing branch April 27, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants