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

Feature/operator splitting #572

Open
wants to merge 147 commits into
base: feature/sunstepper
Choose a base branch
from

Conversation

Steven-Roberts
Copy link
Collaborator

No description provided.

Comment on lines +57 to +60
#include <arkode/arkode_sprkstep.h> // SPRKStep provides symplectic partitioned RK methods.
#include <arkode/arkode_splittingstep.h> // SplittingStep provides operator splitting methods.
#include <arkode/arkode_forcingstep.h> // ForcingStep provides a forcing method.
#include <arkode/arkode_mristep.h> // MRIStep provides multirate RK methods.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there some logic behind moving MRIStep to the end of this list? In the absence of some real reason for doing this, I think that this should retain the chronological ordering, with the new steppers put at the end of the list.

Comment on lines +50 to +55
(:numref:`ARKODE.Usage.ARKStep.UserCallable`,
:numref:`ARKODE.Usage.ERKStep.UserCallable`,
:numref:`ARKODE.Usage.SplittingStep.UserCallable`,
:numref:`ARKODE.Usage.ForcingStep.UserCallable`,
:numref:`ARKODE.Usage.MRIStep.UserCallable`,
and :numref:`ARKODE.Usage.SPRKStep.UserCallable`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there some logic behind inserting the new steppers in the middle of this list? In the absence of some clear justification, I think that this should retain the chronological ordering, with the new steppers put at the end of the list.

Comment on lines +40 to +43
:ref:`ERKStep <ARKODE.Usage.ERKStep>`, :ref:`SPRKStep <ARKODE.Usage.SPRKStep>`,
:ref:`SplittingStep <ARKODE.Usage.SplittingStep>`,
:ref:`ForcingStep <ARKODE.Usage.ForcingStep>`, and
:ref:`MRIStep <ARKODE.Usage.MRIStep>`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there some logic behind inserting the new steppers in the middle of this list? In the absence of some clear justification, I think that this should retain the chronological ordering, with the new steppers put at the end of the list, and MRIStep before SPRKStep.

Comment on lines 82 to 85
SPRKStep/index.rst
SplittingStep/index.rst
ForcingStep/index.rst
MRIStep/index.rst
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there some logic behind inserting the new steppers in the middle of this list? In the absence of some clear justification, I think that this should retain the chronological ordering, with the new steppers put at the end of the list, and MRIStep before SPRKStep.

examples/arkode/C_serial/ark_analytic_partitioned.c Outdated Show resolved Hide resolved
Comment on lines +577 to +581
.. _ARKODE.Mathematics.SplittingStep:

SplittingStep -- Operator splitting methods
================================================

Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar question as elsewhere -- why were the new steppers inserted before MRIStep? As everywhere else, I see no reason to jumble the order. These should be listed chronologically: ARKStep, ERKStep, MRIStep, SPRKStep, SplittingStep/ForcingStep/LSRKStep, ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It made sense to group MRI, splitting, and forcing in the docs and I put the new methods first since they are simpler. I can switch to chronological, but I'll note many place put SPRKStep before MRIStep. Should I reorder SPRKStep while I'm at it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can discuss this as a group, but I think that ordering them either chronologically or most-to-least-used would be optimal. In the latter case, we have multiple applications who use ARKStep, and a few who use ERKStep and MRIStep (with more coming). I see no reason to insert new and currently-unused steppers in front, forcing users to scroll down to get to the documentation that they need.

@drreynolds drreynolds mentioned this pull request Oct 2, 2024
@Steven-Roberts
Copy link
Collaborator Author

In preparation of #587 being merged, I'm wondering how to best count RHS evals. Currently, it can't be accessed through the SUNStepper API, and for some implementations, e.g., exact solution, no RHS may even be used. I see two options:

  • Add new SUNStepper_SetGetNumRhsEvalsFn and SUNStepper_GetNumRhsEvals functions and use these in ARKodeGetNumRhsEvals
  • Have ARKodeGetNumRhsEvals return an error or 0 evals.

I suppose I favor the first option. Anyone else have ideas or preferences?

@drreynolds
Copy link
Collaborator

In preparation of #587 being merged, I'm wondering how to best count RHS evals. Currently, it can't be accessed through the SUNStepper API, and for some implementations, e.g., exact solution, no RHS may even be used. I see two options:

  • Add new SUNStepper_SetGetNumRhsEvalsFn and SUNStepper_GetNumRhsEvals functions and use these in ARKodeGetNumRhsEvals
  • Have ARKodeGetNumRhsEvals return an error or 0 evals.

I suppose I favor the first option. Anyone else have ideas or preferences?

I think that you could leave it out entirely, and ARKODE will then return an error that the function is unsupported. If a user creates the SUNStepper components out of SUNDIALS integrators, then they can get the number of RHS evaluations for each component through those directly, and if they provide their own SUNStepper, then they can query that if they want to know what happened internally.

@Steven-Roberts
Copy link
Collaborator Author

I think that you could leave it out entirely, and ARKODE will then return an error that the function is unsupported.

Ok, that sounds reasonable. On further consideration, my original idea of getting RHS evals from the SUNStepper becomes very complicated if nested partitioning is used. Probably best to avoid that.

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.

5 participants