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

SUNStepper basics based on MRIStepInnerStepper #463

Merged
merged 103 commits into from
Nov 22, 2024
Merged

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.

src/arkode/arkode.c Outdated Show resolved Hide resolved
src/arkode/arkode_arkstep.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper_impl.h Outdated Show resolved Hide resolved
@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).

src/arkode/arkode_arkstep.c Outdated Show resolved Hide resolved
src/arkode/xbraid/arkode_xbraid.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
include/sundials/sundials_stepper.h Show resolved Hide resolved
include/sundials/sundials_stepper.h Outdated Show resolved Hide resolved
include/sundials/sundials_stepper.h Show resolved Hide resolved
include/sundials/sundials_stepper.h Show resolved Hide resolved
src/arkode/arkode_arkstep.c Outdated Show resolved Hide resolved
src/arkode/arkode_arkstep.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
src/arkode/arkode_arkstep.c Outdated Show resolved Hide resolved
include/sundials/sundials_stepper.h Show resolved Hide resolved
src/arkode/arkode_arkstep.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Show resolved Hide resolved
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

src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
src/sundials/sundials_stepper_impl.h Show resolved Hide resolved
include/sundials/sundials_stepper.h Outdated Show resolved Hide resolved
src/sundials/sundials_stepper.c Outdated Show resolved Hide resolved
include/sundials/sundials_stepper.h Outdated Show resolved Hide resolved
@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

doc/shared/sunstepper/SUNStepper_Description.rst Outdated Show resolved Hide resolved
doc/shared/sunstepper/SUNStepper_Description.rst Outdated Show resolved Hide resolved
include/sundials/sundials_stepper.h Show resolved Hide resolved
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.

src/arkode/arkode_sunstepper.c Outdated Show resolved Hide resolved
@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
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