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

Add Generic Rosenbrock Algorithm #61

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Add Generic Rosenbrock Algorithm #61

wants to merge 21 commits into from

Conversation

dennisYatunin
Copy link
Member

PULL REQUEST

Purpose and Content

This PR adds Rosenbrock methods to ClimaTimeSteppers. Some of these methods can work with limiters (depending on the method coefficients), but that requires specifying multiply! and set_Δtγ! in addition to linsolve!. The Rosenbrock methods support updating the Jacobian once per timestep, or once every n timesteps (the latter is not possible with OrdinaryDiffEq).

Benefits and Risks

This PR will allow us to compare the performance of our Rosenbrock23 implementation to that of OrdinaryDiffEq's. The new methods have been tested with convergence unit tests on a simple linear problem (those that support the increment formulation in addition to the tendency formulation have been tested with ForwardEulerODEFunction as well). This PR also involves adding some new tests from ARKode, as requested by Tapio, but those are not properly working yet.

PR Checklist

  • This PR has a corresponding issue OR is linked to an SDI.
  • I have followed CliMA's codebase contribution and style guidelines OR N/A.
  • I have followed CliMA's documentation policy.
  • I have checked all issues and PRs and I certify that this PR does not duplicate an open PR.
  • I linted my code on my local machine prior to submission OR N/A.
  • Unit tests are included OR N/A.
  • Code used in an integration test OR N/A.
  • All tests ran successfully on my local machine OR N/A.
  • All classes, modules, and function contain docstrings OR N/A.
  • Documentation has been added/updated OR N/A.

@bischtob bischtob requested a review from simonbyrne September 21, 2022 16:21

struct RosenbrockTableau{N, RT, N²}
Copy link
Member

Choose a reason for hiding this comment

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

Wait, we already had a Rosenbrock time stepper? How is what was added different? Can this please be elaborated in the description? The previous implementation (that it looks like @simonbyrne implemented?) looks much simpler

Copy link

Choose a reason for hiding this comment

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

I have a similar question, perhaps phrased slightly differently: Why does enabling limiters necessitate this much more complexity than the straightforward Rosenbrock implementation in the older code?

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.

3 participants