-
Notifications
You must be signed in to change notification settings - Fork 194
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
Remove tendency computation at model construction #3741
Remove tendency computation at model construction #3741
Conversation
src/Models/HydrostaticFreeSurfaceModels/update_hydrostatic_free_surface_model_state.jl
Outdated
Show resolved
Hide resolved
src/Models/HydrostaticFreeSurfaceModels/update_hydrostatic_free_surface_model_state.jl
Outdated
Show resolved
Hide resolved
src/Models/NonhydrostaticModels/update_nonhydrostatic_model_state.jl
Outdated
Show resolved
Hide resolved
The logic should be reversed, we should have to include "compute_tendencies=false" as an optimization. Rather than what's implemented here, which does the unsafe thing by default. The problem with this logic is that it makes it harder to implement new models. The optimizations should be the optional thing basically, if one is naive, then things should work even if they are slower than they could be |
|
||
compute_tendencies && | ||
@apply_regionally compute_tendencies!(model, callbacks) |
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 just noticed that the SingleColumnGrid
was never computing the tendencies. Is this a bug? Was this mode working? I can add a test for it here
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.
Found out the issue, because of this line
Lines 28 to 30 in 5a3d3c9
update_state!(model::HydrostaticFreeSurfaceModel, callbacks=[]; compute_tendencies = true) = | |
update_state!(model, model.grid, callbacks; compute_tendencies) | |
the update_state!
in this file was neglected because of the lack of the compute_tendencies
keyword argument. So, apparently, it is a bug, but one that only influences performance and not results
…ate.jl Co-authored-by: Gregory L. Wagner <[email protected]>
This should be ready to merge |
At the moment, the tendencies were updated at each call of
update_state!
. This PR makes it so that the tendencies are computed only inside the time-stepping where we need them.see ClimaOcean#164