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.

Sign up for GitHub

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

Jump to bottom

Make it easier to restart a simulation from a checkpoint with additional passive tracers #2938

Merged
merged 2 commits into from
Feb 23, 2023
Merged

Make it easier to restart a simulation from a checkpoint with additional passive tracers #2938

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a91eea
rethrow error if it's not the expected type
tomchor Feb 22, 2023
58cadff
change from try-catch to if-else
tomchor Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions src/OutputWriters/checkpointer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,12 @@ function set!(model, filepath::AbstractString)
parent_data = file["$name/data"]
model_field = model_fields[name]
copyto!(model_field.data.parent, parent_data)
catch
@warn "Could not retore $name from checkpoint."
catch err
if err isa KeyError
Copy link
Member

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?

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Member

@glwagner glwagner Feb 23, 2023 •
edited
Loading

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).

@warn "Could not restore $name from checkpoint."
else
rethrow(err)
end
end
end

Expand All @@ -236,17 +240,25 @@ end

function set_time_stepper_tendencies!(timestepper, file, model_fields)
for name in propertynames(model_fields)
# Tendency "n"
parent_data = file["timestepper/Gⁿ/$name/data"]

tendencyⁿ_field = timestepper.Gⁿ[name]
copyto!(tendencyⁿ_field.data.parent, parent_data)

# Tendency "n-1"
parent_data = file["timestepper/G⁻/$name/data"]

tendency⁻_field = timestepper.G⁻[name]
copyto!(tendency⁻_field.data.parent, parent_data)
try
# Tendency "n"
parent_data = file["timestepper/Gⁿ/$name/data"]

tendencyⁿ_field = timestepper.Gⁿ[name]
copyto!(tendencyⁿ_field.data.parent, parent_data)

# Tendency "n-1"
parent_data = file["timestepper/G⁻/$name/data"]

tendency⁻_field = timestepper.G⁻[name]
copyto!(tendency⁻_field.data.parent, parent_data)
catch err
if err isa KeyError
tomchor marked this conversation as resolved.
Show resolved Hide resolved
@warn "Could not restore tendencies for $name from checkpoint."
else
rethrow(err)
end
end
end

return nothing
Expand Down