SUNStepper basics based on MRIStepInnerStepper #463

balos1 · 2024-04-29T22:37:04Z

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

src/arkode/arkode_arkstep.c

src/sundials/sundials_stepper.c

src/sundials/sundials_stepper_impl.h

balos1 · 2024-05-06T18:40:38Z

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

drreynolds

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

src/arkode/arkode_arkstep.c

src/arkode/xbraid/arkode_xbraid.c

src/sundials/sundials_stepper.c

include/sundials/sundials_stepper.h

src/arkode/arkode_arkstep.c

src/sundials/sundials_stepper.c

src/arkode/arkode_arkstep.c

include/sundials/sundials_stepper.h

src/arkode/arkode_arkstep.c

src/sundials/sundials_stepper.c

Steven-Roberts

Initial pass at latest changes

src/sundials/sundials_stepper.c

src/sundials/sundials_stepper_impl.h

include/sundials/sundials_stepper.h

src/sundials/sundials_stepper.c

include/sundials/sundials_stepper.h

Steven-Roberts · 2024-08-21T00:29:51Z

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.

…ature/sunstepper

Steven-Roberts · 2024-10-02T23:49:49Z

src/arkode/arkode_sunstepper.c

+  ARKodeMem ark_mem = (ARKodeMem)arkode_mem;
+
+  SUNErrCode err = SUNStepper_Create(ark_mem->sunctx, stepper);
+  if (err != SUN_SUCCESS) { return ARK_SUNSTEPPER_ERR; }


Should this invoke arkProcessError?

I don't feel strongly on this question either way. While it is an ARKODE-provided routine (and so arkProcessError makes sense), it is a utility routine provided for SUNStepper users, so I could equivalently see it returning a SUNErrCode instead of an int, and "feeling" more similar to the SUNStepper routines.

swig/sundials/fsundials_stepper.i

Steven-Roberts · 2024-10-02T23:57:00Z

src/sundials/sundials_stepper_impl.h

+  SUNStepperFullRhsFn fullrhs;
+  SUNStepperResetFn reset;
+  SUNStepperSetStopTimeFn setstoptime;
+  SUNStepperSetForcingFn setforcing;


Note for reviewers: unlike MRIStepInnerStepper, SUNStepper does not include properties for forcing. It defers that to the implementation. This streamlines this struct, removes the need for a forcing "getter," and can reduce storage (saves a N_Vector for ForcingStep over previous version).

Steven-Roberts · 2024-10-03T00:01:06Z

doc/shared/sunstepper/SUNStepper_Description.rst

+.. _SUNStepper.Description.BaseMethods.SteppingFunctions:
+
+Stepping Functions
+^^^^^^^^^^^^^^^^^^


Note for reviewers: Unlike MRIStepInnerStepper, the functions in this section are publicly documented. They can be hidden if folks think that's preferable.

I think that these functions should be publicly documented, in case a user wishes to construct a custom SUNStepper. I'm particularly thinking of operator-splitting methods where analytical solutions exist for some partitions, in which case each of these would be easy to implement but may have subtle differences.

Steven-Roberts · 2024-10-03T00:02:12Z

doc/shared/sunstepper/SUNStepper_Description.rst

+Attaching and Accessing the Content Pointer
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+.. c:function:: SUNErrCode SUNStepper_SetContent(SUNStepper stepper, void *content)


Note for reviewers: these functions are new to SUNStepper compared to MRIStepInnerStepper. Since the SUNStepper struct is private, I think this is the proper way to access the last_flag.

Steven-Roberts · 2024-10-03T00:38:36Z

include/sundials/sundials_stepper.h

+SUNErrCode SUNStepper_SetOneStepFn(SUNStepper stepper, SUNStepperOneStepFn fn);
+
+SUNDIALS_EXPORT
+SUNErrCode SUNStepper_SetFullRhsFn(SUNStepper stepper, SUNStepperFullRhsFn fn);


Note to reviewers: there is no analogous SUNStepper_FullRhs. This is following what MRI does where it was presumably excluded because the full RHS functions are private in ARKODE. I can make a public SUNStepper_FullRhs function if folks want.

Steven-Roberts · 2024-10-03T00:42:52Z

doc/shared/sunstepper/SUNStepper_Description.rst

+This section describes the virtual methods defined by the :c:type:`SUNStepper`
+abstract base class.
+
+An :c:type:`SUNStepper` *must* provide implementations of the following


I've updated the docs to indicate they are optional and determined by the "consumer" of the SUNStepper. I'll need to specify the required functionality in the operator splitting PR.

…ature/sunstepper

drreynolds

A few minor items, but once those are resolved I think this is good to merge.

drreynolds · 2024-10-03T13:11:33Z

doc/shared/sunstepper/SUNStepper_Description.rst

+.. _SUNStepper.Description.BaseMethods.SteppingFunctions:
+
+Stepping Functions
+^^^^^^^^^^^^^^^^^^


I think that these functions should be publicly documented, in case a user wishes to construct a custom SUNStepper. I'm particularly thinking of operator-splitting methods where analytical solutions exist for some partitions, in which case each of these would be easy to implement but may have subtle differences.

doc/arkode/guide/source/Usage/MRIStep/Custom_Inner_Stepper/Description.rst

drreynolds · 2024-10-14T13:15:27Z

doc/arkode/guide/source/Usage/User_callable.rst

+.. c:function:: int ARKodeCreateSUNStepper(void *inner_arkode_mem, SUNStepper *stepper)
+
+   Wraps an ARKODE integrator as a :c:type:`SUNStepper`.
+
+   :param arkode_mem: pointer to the ARKODE memory block.
+   :param stepper: the :c:type:`SUNStepper` object.
+
+   :retval ARK_SUCCESS: the function exited successfully.
+   :retval ARK_MEM_FAIL: a memory allocation failed.
+   :retval ARK_SUNSTEPPER_ERR: the :c:type:`SUNStepper` initialization failed.
+
+   .. versionadded:: x.y.z


Are there any assumed requirements about the ARKODE solver module? Specifically, since some steppers "support" all ARKODE functionality, whereas other steppers only support a tiny subset, then it would be good to clarify which steppers this function would work for.

The only functionality not supported by all of ARKODE is SUNStepper_SetForcing. I'll make note of that here and in ARKStep, ERKStep, etc.

doc/shared/sunstepper/SUNStepper_Description.rst

drreynolds · 2024-10-14T13:22:19Z

doc/shared/sunstepper/SUNStepper_Implementing.rst

+Implementing a SUNStepper
+=========================


It would be great if we had an example that implemented a custom SUNStepper, in which case this file could also mention an example where users can see these instructions "in action." I'm imagining a simple operator-splitting example where one partition has an analytical solution.

In the operator splitting PR, currently only a unit test creates a custom SUNStepper. I can see about modifying the advection-diffusion-reaction example so we have some something to point to.

Or this could be added in the near future (not required for approval of this PR).

drreynolds · 2024-10-14T13:28:50Z

src/arkode/arkode_sunstepper.c

+  ARKodeMem ark_mem = (ARKodeMem)arkode_mem;
+
+  SUNErrCode err = SUNStepper_Create(ark_mem->sunctx, stepper);
+  if (err != SUN_SUCCESS) { return ARK_SUNSTEPPER_ERR; }


I don't feel strongly on this question either way. While it is an ARKODE-provided routine (and so arkProcessError makes sense), it is a utility routine provided for SUNStepper users, so I could equivalently see it returning a SUNErrCode instead of an int, and "feeling" more similar to the SUNStepper routines.

Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu>

…tion

balos1 assigned Steven-Roberts Apr 29, 2024

drreynolds reviewed Apr 30, 2024

View reviewed changes

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 changed the title ~~move MRIStepInnerStepper to SUNStepper~~ SUNStepper basics based on MRIStepInnerStepper May 6, 2024

drreynolds reviewed May 6, 2024

View reviewed changes

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

gardner48 reviewed May 12, 2024

View reviewed changes

Steven-Roberts reviewed May 13, 2024

View reviewed changes

gardner48 force-pushed the develop branch from 37f4a07 to 23581e8 Compare June 20, 2024 03:39

balos1 added 9 commits June 20, 2024 16:58

move MRIStepInnerStepper to SUNStepper

1267d97

add advance, onestep, trystep functions

19f921c

revert arkode changes

ca7768e

implement Advance, OneStep, and TryStep

bf3d836

cleanup

b96f1ee

wrap SUNStepper as MRIStepInnerStepper

e87e70f

update sunstepper evolve functions to pass tret and return reason

86db0fe

minor fixes

02fcf40

rebase fix

c04f24a

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

balos1 added 8 commits July 1, 2024 15:08

Merge remote-tracking branch 'origin/develop' into feature/sunstepper

f9ff00c

Merge remote-tracking branch 'origin/develop' into feature/sunstepper

49e902c

Merge remote-tracking branch 'origin/develop' into feature/sunstepper

4be592c

bring up to date

1a92308

move trystep flahs

3c3a406

cleanup

cbfbd79

add yp arg

e6815d6

add setstoptime

c7d3f39

Steven-Roberts reviewed Aug 20, 2024

View reviewed changes

balos1 added 2 commits August 22, 2024 09:44

minor fixes

b7d7e08

add missing SetForcing declaration

491765f

Steven-Roberts and others added 17 commits October 2, 2024 14:16

Merge branch 'develop' into feature/sunstepper

01eba9e

Reference ARKodeCreateSUNStepper in the SUNStepper docs

13e444e

Revert ops check change

03e26d6

Mention MRIStepInnerStepper_CreateFromSUNStepper in changelog

9f6baab

Typo fixes

b1a3dad

Remove extra include

50cb5a1

more cleanup

ec8bbde

Move function to make diff nicer

2a05e5d

Improve error handling for MRI inner stepper wrapping SUNStepper

92a7f17

document constant that was added

24114fe

Merge branch 'feature/sunstepper' of github.com:LLNL/sundials into fe…

ef116d5

…ature/sunstepper

Remove SUNStepper_TryStep

68e1d1e

Revert trystep changes for xbraid

e24ed27

Apply formatter

aa7ccc1

Typo fix

ec41dfa

Add missing period

106022a

Make return consistent

81fa70f

Steven-Roberts reviewed Oct 3, 2024

View reviewed changes

Steven-Roberts added 4 commits October 3, 2024 15:40

Update swig/sundials/fsundials_stepper.i

9e95184

Doc style fixes

7158562

Merge branch 'feature/sunstepper' of github.com:LLNL/sundials into fe…

f9dc452

…ature/sunstepper

Merge branch 'develop' into feature/sunstepper

926caa0

Steven-Roberts requested a review from drreynolds October 10, 2024 18:28

drreynolds reviewed Oct 14, 2024

View reviewed changes

Steven-Roberts and others added 6 commits October 14, 2024 09:34

Apply suggestions from code review

a17092a

Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu>

Fix copy paste error in MRIStepInnerStepper_CreateFromSUNStepper example

13c7c63

Only call SUNStepper_SetForcingFn if stepper provides setforcing func…

c333f57

…tion

Document support for forcing

d4846b3

Merge branch 'develop' into feature/sunstepper

cb495dc

Apply formatter

9641457

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SUNStepper basics based on MRIStepInnerStepper #463

SUNStepper basics based on MRIStepInnerStepper #463

balos1 commented Apr 29, 2024

balos1 commented May 6, 2024

drreynolds left a comment

Steven-Roberts left a comment

Steven-Roberts commented Aug 21, 2024 •

edited

Loading

Steven-Roberts Oct 2, 2024

drreynolds Oct 14, 2024

Steven-Roberts Oct 2, 2024

Steven-Roberts Oct 3, 2024

drreynolds Oct 3, 2024

Steven-Roberts Oct 3, 2024

Steven-Roberts Oct 3, 2024

Steven-Roberts Oct 3, 2024

drreynolds left a comment

drreynolds Oct 3, 2024

drreynolds Oct 14, 2024

Steven-Roberts Oct 14, 2024

drreynolds Oct 14, 2024

Steven-Roberts Oct 14, 2024

drreynolds Oct 14, 2024

drreynolds Oct 14, 2024

		Implementing a SUNStepper
		=========================

SUNStepper basics based on MRIStepInnerStepper #463

Are you sure you want to change the base?

SUNStepper basics based on MRIStepInnerStepper #463

Conversation

balos1 commented Apr 29, 2024

balos1 commented May 6, 2024

drreynolds left a comment

Choose a reason for hiding this comment

Steven-Roberts left a comment

Choose a reason for hiding this comment

Steven-Roberts commented Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drreynolds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Steven-Roberts commented Aug 21, 2024 •

edited

Loading