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

Change post to pre hooks #323

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Change post to pre hooks #323

wants to merge 1 commit into from

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Nov 8, 2024

This PR is an attempt to close #322 and #270. This PR somewhat reverts (with some tweaks) #222 and #223. And #210 is also related.

This is a breaking change, but I think it's really needed since the existing hooks are both confusingly named and improperly implemented to allow splitting these functions.

@charleskawczynski
Copy link
Member Author

Need to rebase after #333 merges.

@charleskawczynski
Copy link
Member Author

We could actually make this non-breaking by making some small adjustments to the ClimaODEFunction constructor, and just forwarding post_explicit!/post_implicit! keywords to the pre-functions, knowing that users must be using the same function for their code to work correctly anyway.

Copy link
Member

@dennisYatunin dennisYatunin left a comment

Choose a reason for hiding this comment

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

I would advise against using either pre_explicit or post_explicit, as that does not accurately describe what this function will do. The correct name for this function is either post_stage (preferred) or pre_tendency (usually correct, except at the end of the last timestep, since there are no further tendencies to compute after that). The other function can be called pre_implicit, but it would be more correct to call it pre_implicit_solve, since the implicit tendency for each stage will still be computed with post_stage/pre_tendency.

I'm also not really in favor of allowing pre_explicit and pre_implicit to be used as aliases for these new functions, as that will almost certainly lead to confusion and bugs in the future. Those names are not indicative of how these functions will be used, so we should probably just make this a breaking change.

@charleskawczynski
Copy link
Member Author

I would advise against using either pre_explicit or post_explicit, as that does not accurately describe what this function will do. The correct name for this function is either post_stage (preferred) or pre_tendency (usually correct, except at the end of the last timestep, since there are no further tendencies to compute after that). The other function can be called pre_implicit, but it would be more correct to call it pre_implicit_solve, since the implicit tendency for each stage will still be computed with post_stage/pre_tendency.

As I mentioned in #322, post-anything is a bad name in that it's describing the implementation detail, and not what the function actually does-- it pre-computes variables that are shared between multiple function calls with the same state and at the same time (since we know that then, and only then, the cache should be the same). So, I'm now pretty firm on removing post from all of the names.

pre_tendency sounds nice, but also ambiguous, which tendencies are to be shared? Part of what makes exposing these hooks complicated is that they arise from implementation details-- different step! implementations will allow for sharing the cache between multiple function calls in different ways. Perhaps we should see if we can pass functors like SharedCache(wfact!, implicit_tendency!) to ClimaODEFunction, and then that would indicate to the time-stepper that the user could compute the cache for both implicit_tendency! and wfact! with that functor call.

I'm also not really in favor of allowing pre_explicit and pre_implicit to be used as aliases for these new functions, as that will almost certainly lead to confusion and bugs in the future. Those names are not indicative of how these functions will be used, so we should probably just make this a breaking change.

I agree, I was just trying to avoid breaking changes. I think the cleaner thing will be to just fix this bug and make a breaking release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reformulate post_explicit! / post_implicit! hooks
2 participants