-
Notifications
You must be signed in to change notification settings - Fork 190
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
Make it easier to restart a simulation from a checkpoint with additional passive tracers #2938
Conversation
This feature would be used to restart a simulation from a checkpoint with additional passive tracers? Is that correct? |
That is correct. That's what I'm currently trying to do (and eventually @whitleyv too) and it's much easier if with we make this change with the Checkpointer. |
Can we update the PR description to state this goal? "This feature adds support for using a checkpoint file to initialize a model that contains additional passive tracers..." |
Done! |
src/OutputWriters/checkpointer.jl
Outdated
catch | ||
@warn "Could not retore $name from checkpoint." | ||
catch err | ||
if err isa KeyError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why KeyError? Maybe add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the expected error when a variable isn't present in the checkpoint. It's possible to have other errors though, so I think should rethrow()
the error if something else goes wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case your comment made me realize the way it was written probably isn't best practice, so I re-wrote the code using an if-else statement instead of a try-catch. The new code is shorter and (I think) clearer. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's easier to understand.
There's the concept of "Easier to Ask for Forgiveness than Permission" which claims that try/catch is better style esp for dynamic languages.
I think our project and code is unique. So while there are places that so-called "best practices" apply, there are also places where we should do what works best for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downside of "look before you leap" is when we have many error conditions which causes the if-statements to pile up. Here we have just one condition so I think explicit writing the condition is better and more readable (for me).
@@ -209,16 +209,12 @@ function set!(model, filepath::AbstractString) | |||
model_fields = prognostic_fields(model) | |||
|
|||
for name in propertynames(model_fields) | |||
try | |||
if string(name) ∈ keys(file) # Test if variable exist in checkpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this! Easier to understand when reading it
This feature adds support for using a checkpoint file to initialize a model that contains additional passive tracers that weren't present in the original simulation. The use case in mind is whenever a user wants to start passive tracers only after simulation spin-up, for example.
At the moment, on main, this isn't possible since if we try to pickup a simulation but a given variable can't be found in the checkpointer file, the code throws a warning when trying to set the data for that variable, and an error when trying to set its tendencies.
This PR changes the code so that it throws an error for both cases. After this PR a user can then do:
On main this throws a
KeyError
. On this branch this produces:One option to add safety is to modify
set!(model, filepath::AbstractString)
to take an option argumentskip_missing_variables = false
(or something), which would change the behavior from throwing a warning to throwing an error.@whitleyv