Skip to content

Conversation

balos1
Copy link
Member

@balos1 balos1 commented Apr 29, 2024

I am opening this PR as a draft to gather feedback. This renames/moves MRIStepInnerStepper to SUNStepper so that @Steven-Roberts and I can extend it for other purposes.

@balos1
Copy link
Member Author

balos1 commented May 6, 2024

@drreynolds @gardner48 I have made the changes we discussed on Tuesday. Let me know what you think.

@balos1 balos1 changed the title move MRIStepInnerStepper to SUNStepper SUNStepper basics based on MRIStepInnerStepper May 6, 2024
Copy link
Collaborator

@drreynolds drreynolds left a comment

Choose a reason for hiding this comment

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

I like how clean this is now. I noticed a few items for comment (see below).

@balos1 balos1 force-pushed the feature/sunstepper branch from a030d57 to c04f24a Compare June 21, 2024 00:04
Copy link
Collaborator

@Steven-Roberts Steven-Roberts left a comment

Choose a reason for hiding this comment

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

Initial pass at latest changes

@Steven-Roberts
Copy link
Collaborator

Steven-Roberts commented Aug 21, 2024

After some more thought and discussion with David on IDA and passing around yp, the primary issue I see is keeping it consistent with y. With operator splitting, for example, y jumps around between calls to evolve, and I don't see how to keep yp consistent. With MRI, the change in the forcing between stages can cause a similar issue. This is not something the outer methods can do and needs to be done in the inner SunStepper integrator.

I'd like to propose the following to make SunStepper self starting (all you need is y to reset then evolve):

  • Remove yp as arguments
  • To support an IDA SunStepper, have a constructor like
int IDACreateSUNStepper(void* inner_ida_mem, ConsistentYPrimeFn f, SUNStepper* stepper)

where there's a function which can be specified by users (we could have a default that uses IDACalcIC) to get yp given y.

@balos1
Copy link
Member Author

balos1 commented Oct 30, 2024

I actually need the OneStep function in the adjoint work, so I have added it back into here.

@drreynolds drreynolds self-requested a review November 5, 2024 18:11
drreynolds
drreynolds previously approved these changes Nov 5, 2024
@drreynolds
Copy link
Collaborator

It looks like the only remaining issue with the CI is that the documentation updates have some label errors:

/var/lib/jenkins/workspace/SUNDIALS_PR-463/doc/shared/RecentChanges.rst:5: WARNING: undefined label: 'arkode.usage.splittingstep'
/var/lib/jenkins/workspace/SUNDIALS_PR-463/doc/shared/RecentChanges.rst:5: WARNING: undefined label: 'arkode.usage.forcingstep'
/var/lib/jenkins/workspace/SUNDIALS_PR-463/doc/shared/sunstepper/SUNStepper_Implementing.rst:42: WARNING: c:func reference target not found: SplittingStepCreate
/var/lib/jenkins/workspace/SUNDIALS_PR-463/doc/shared/sunstepper/SUNStepper_Implementing.rst:42: WARNING: c:func reference target not found: ForcingStepCreate
/var/lib/jenkins/workspace/SUNDIALS_PR-463/doc/shared/sunstepper/SUNStepper_Implementing.rst:55: WARNING: c:func reference target not found: SplittingStepCreate
/var/lib/jenkins/workspace/SUNDIALS_PR-463/doc/shared/sunstepper/SUNStepper_Implementing.rst:55: WARNING: c:func reference target not found: ForcingStepCreate
/var/lib/jenkins/workspace/SUNDIALS_PR-463/doc/shared/RecentChanges.rst:5: WARNING: undefined label: 'arkode.usage.splittingstep'
/var/lib/jenkins/workspace/SUNDIALS_PR-463/doc/shared/RecentChanges.rst:5: WARNING: undefined label: 'arkode.usage.forcingstep'

@Steven-Roberts
Copy link
Collaborator

Yeah, wasn't sure how to best handle that. Those are references to parts of the docs that will be added in the operator splitting PR (which is merged after this). I can temporarily comment out or remove the references out to get the CI passing.

@drreynolds
Copy link
Collaborator

Yeah, wasn't sure how to best handle that. Those are references to parts of the docs that will be added in the operator splitting PR (which is merged after this). I can temporarily comment out or remove the references out to get the CI passing.

I forget, but is the "merge" plan to approve/merge this PR before #572? If that's the case, then perhaps we should move these bits of documentation into #572 so that both "squashed" and merged PRs can pass all CI tests?

@Steven-Roberts
Copy link
Collaborator

I forget, but is the "merge" plan to approve/merge this PR before #572?

Yes

If that's the case, then perhaps we should move these bits of documentation into #572 so that both "squashed" and merged PRs can pass all CI tests?

Ok, I'll do that

drreynolds
drreynolds previously approved these changes Nov 15, 2024
Copy link
Member

@gardner48 gardner48 left a comment

Choose a reason for hiding this comment

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

One minor change (missing process error call) that I'll apply and approve in a sec.

@gardner48 gardner48 merged commit 7320d08 into develop Nov 22, 2024
30 checks passed
@gardner48 gardner48 deleted the feature/sunstepper branch November 22, 2024 01:58
gardner48 added a commit that referenced this pull request Jul 21, 2025
Add SUNStepper class for evolving IVPs

---------

Co-authored-by: Steven Roberts <roberts115@llnl.gov>
Co-authored-by: David Gardner <gardner48@llnl.gov>
Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu>
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.

4 participants