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

Remove tendency computation at model construction #3741

Merged

Conversation

simone-silvestri
Copy link
Collaborator

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

@glwagner
Copy link
Member

glwagner commented Aug 27, 2024

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

Comment on lines +81 to +83

compute_tendencies &&
@apply_regionally compute_tendencies!(model, callbacks)
Copy link
Collaborator Author

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

Copy link
Collaborator Author

@simone-silvestri simone-silvestri Aug 29, 2024

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

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

@simone-silvestri
Copy link
Collaborator Author

This should be ready to merge

@simone-silvestri simone-silvestri merged commit bf04295 into main Aug 30, 2024
46 checks passed
@simone-silvestri simone-silvestri deleted the ss/remove-compute-tendencies-at-model-construction branch August 30, 2024 17:35
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.

2 participants