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

Reformulate post_explicit! / post_implicit! hooks #322

Open
charleskawczynski opened this issue Nov 7, 2024 · 1 comment · May be fixed by #323
Open

Reformulate post_explicit! / post_implicit! hooks #322

charleskawczynski opened this issue Nov 7, 2024 · 1 comment · May be fixed by #323
Labels
bug Something isn't working

Comments

@charleskawczynski
Copy link
Member

There are two fundamental issues with our post_explicit! and post_implicit! hooks:

  • First, the name is bad in that they convey when in the time marching algorithm they are called, and not what they are actually used for, which is to precompute variables that are shared between multiple function calls.

  • Second, the implementation is wrong in that it does not allow users to supply different functions to post_explicit! and post_implicit!. Concretely, post_explcit! precomputes variables for upcoming implicit stages, before they are run, but post_explcit! is also called at the very end of the timestep, in preparation for precomputing variables in the callbacks, and the explicit stage of the next timestep. Therefore, it must contain precomputed quantities for both implicit and explicit. The post_implicit! is called after every newton iteration, which means that it must precompute variables for the next implicit newton iteration, as well as the final explicit update. Therefore, post_implicit! must also contain precomputed quantities for both implicit and explicit.

This is very unfortunate because while the implementation is correct when post_explicit! and post_explicit! point to the same function that precompute all variables, and the change to this implementation allowed us to reduce the number of total precompute calls by 1, it also resulted in us not being able to split these calls, which could have a more significant impact, especially for implicit simulations with multiple newton iterations.

@charleskawczynski charleskawczynski added the bug Something isn't working label Nov 7, 2024
@charleskawczynski
Copy link
Member Author

What I propose we do is:

  • Change all post_-hooks to precompute_-hooks, and appropriately put 1 precompute hook before each explicit / implicit set of calls-- should be easy.
  • Add one additional precompute_explicit! to the very end of the timestep, and conditionally remove the first precompute_explicit! call, based on whether it's the first step or not. This way, we still have the same number of calls (the last precompute_explicit! will allow us to share the precomputed variables in the callbacks + the beginning of the next timestep).

@charleskawczynski charleskawczynski linked a pull request Nov 8, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant