Feature/arkode sts #541

maggul · 2024-07-12T15:27:12Z

This is a draft PR for feedback on new STS module.

drreynolds · 2024-07-12T15:57:14Z

Unfortunately, the rebase to develop seems to have marked hundreds of files as conflicts. I'll work on fixing this branch now, so that it will be easier for folks to review.

…g module

balos1

OK, I have made it through the documentation.

doc/arkode/guide/source/Constants.rst

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

balos1 · 2024-09-17T16:59:36Z

doc/arkode/guide/source/Mathematics.rst

+============================================
+
+The LSRKStep time-stepping module in ARKODE supports a variety of so-called
+"low-storage" Runge--Kutta methods.  This category includes traditional explicit 


Can you cite some references for these different types of low-storage methods?

doc/arkode/guide/source/Mathematics.rst

balos1 · 2024-09-17T17:05:11Z

doc/arkode/guide/source/Usage/LSRKStep/User_supplied.rst

+
+When running LSRKStep with either the RKC or RKL methods, the user must supply a dominant eigenvalue estimation function of type :c:type:`ARKDomEigFn`:
+
+.. c:type:: int (*ARKDomEigFn)(sunrealtype* t, N_Vector y, sunrealtype* lambdaR, sunrealtype* lambdaI, void* user_data)


Since we plan on adding complex support later with suncomplextype, I wonder if we should add suncomplextype to sundials_types.h now so it can be used in this function (instead of having a lambdaR and lambdaI). What do you think @gardner48 @drreynolds ?

we need to use this even in the real valued problems, suncomplextype might not be appropriate

…or sod shock problem

Steven-Roberts

Reviewed the first half or so of the files. Many of my comments are on inconsistent naming conventions, and I made no attempt to catch all but enough to show the patterns.

You'll also want to update this to add the header info:

sundials/doc/arkode/guide/source/Usage/General.rst

Line 54 in c28eaa3

#include <sundials/sundials_core.h> // Provides core SUNDIALS types

Steven-Roberts · 2024-09-17T19:01:22Z

doc/arkode/guide/source/Constants.rst

+   +-----------------------------------------------+------------------------------------------------------------+
+   | :index:`ARKODE_LSRK_RKL`                      | 2nd order Runge-Kutta-Legendre (RKL) method.               |
+   +-----------------------------------------------+------------------------------------------------------------+
+   | :index:`ARKODE_LSRK_SSPs_2`                   | Optimal 2nd order s-stage SSP RK method.                   |


Should this value be all caps?

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

doc/arkode/guide/source/Constants.rst

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

examples/arkode/CXX_parallel/ark_heat2D_lsrk_p.cpp

examples/arkode/C_serial/lsrk_analytic_VarJac.c

examples/arkode/C_serial/lsrk_analytic.c

Steven-Roberts · 2024-09-17T20:30:20Z

include/arkode/arkode_lsrkstep.h

+{
+  ARKODE_LSRK_RKC     = 1, /* ensure enum is int */
+  ARKODE_LSRK_RKL     = 2,
+  ARKODE_LSRK_RKG     = 3,


That's fine since it will be undocumented presumably. I don't see why the particular value matters.

Steven-Roberts · 2024-09-19T17:17:00Z

src/arkode/arkode_sprkstep.c

@@ -151,7 +151,7 @@ void* SPRKStepCreate(ARKRhsFn f1, ARKRhsFn f2, sunrealtype t0, N_Vector y0,

  /* SPRKStep uses Lagrange interpolation by default, since Hermite is
     less compatible with these methods. */
-  ARKodeSetInterpolantType(ark_mem, ARK_INTERP_LAGRANGE);
+  ark_mem->interp_type = ARK_INTERP_LAGRANGE;


Is this change supposed to be here? I'm not sure if we settled on the way to set the default interpolant (prior to fix for #569), but we should be consistent.

Steven-Roberts · 2024-09-19T17:20:14Z

src/arkode/arkode_lsrkstep_io.c

+  {
+  case ARKODE_LSRK_RKC_2:
+    ark_mem->step          = lsrkStep_TakeStepRKC;
+    step_mem->LSRKmethod   = ARKODE_LSRK_RKC_2;


This can be brought out of the switch to step_mem->LSRKmethod = method. These are common sources of copy paste errors.

Steven-Roberts · 2024-09-19T17:29:30Z

src/arkode/arkode_lsrkstep_io.c

+  if (dom_eig != NULL)
+  {
+    step_mem->isextDomEig = SUNTRUE;
+    step_mem->extDomEig   = dom_eig;
+
+    return (ARK_SUCCESS);
+  }
+  else
+  {
+    step_mem->isextDomEig = SUNFALSE;
+    step_mem->extDomEig   = NULL;
+
+    arkProcessError(ark_mem, ARK_ILL_INPUT, __LINE__, __func__, __FILE__,
+                    "Internal dom_eig is not supported yet!");
+    return (ARK_ILL_INPUT);
+  }


Suggested change

if (dom_eig != NULL)

{

step_mem->isextDomEig = SUNTRUE;

step_mem->extDomEig = dom_eig;

return (ARK_SUCCESS);

}

else

{

step_mem->isextDomEig = SUNFALSE;

step_mem->extDomEig = NULL;

arkProcessError(ark_mem, ARK_ILL_INPUT, __LINE__, __func__, __FILE__,

"Internal dom_eig is not supported yet!");

return (ARK_ILL_INPUT);

}

step_mem->isextDomEig = dom_eig != NULL;

step_mem->extDomEig = dom_eig;

if (dom_eig == NULL)

{

arkProcessError(ark_mem, ARK_ILL_INPUT, __LINE__, __func__, __FILE__,

"Internal dom_eig is not supported yet!");

return (ARK_ILL_INPUT);

}

src/arkode/arkode_lsrkstep_io.c

Steven-Roberts · 2024-09-19T17:31:49Z

src/arkode/arkode_lsrkstep_io.c

+    step_mem->constJac   = SUNTRUE;
+    step_mem->domeigfreq = 1;
+  }
+  else { step_mem->domeigfreq = nsteps; }


Should this branch set step_mem->constJac = SUNFALSE?

Steven-Roberts · 2024-09-19T18:24:39Z

src/arkode/arkode_lsrkstep_impl.h

+  /* LSRK method storage and parameters */
+  // N_Vector  temp1; /* Temp vector storage */
+  // N_Vector  temp2; /* Temp vector storage */
+  N_Vector* Fe; /* RHS vector storage */


While this is an array of vectors, only one element is ever used. Can this be replaced with an NVector or is this to support future extensions?

src/arkode/arkode_lsrkstep.c

Steven-Roberts · 2024-09-19T18:31:53Z

src/arkode/arkode_lsrkstep.c

+  const sunrealtype onep54 = SUN_RCONST(1.54), c13 = SUN_RCONST(13.0),
+                    p8 = SUN_RCONST(0.8), p4 = SUN_RCONST(0.4);


A citation on where the come from would be helpful

src/arkode/arkode_lsrkstep.c

…ature/arkode-sts

Steven-Roberts

Finished reviewing the remaining files. Apologies if I was too pedantic on some of the things not specific to this branch.

src/arkode/arkode_lsrkstep.c

Steven-Roberts · 2024-09-19T21:56:22Z

src/arkode/arkode_lsrkstep.c

+    }
+    if (ss == step_mem->stagemaxlimit)
+    {
+      hmax = SUN_RCONST(0.95) * (SUNSQR(ss) + ss - 2) / (2.0 * step_mem->sprad);


Idea: use hadapt_mem->safety instead of 0.95

To that end, we need to create a new double in the step memory and assign its value from hadapt_mem->safety when both of their memories are available (probably in lsrkStep_SetDefaults). Do you think we should have a new step_mem->safety kind of parameter, or keep it in this way?

On second thought, sprad already has the domeigsfty applied, so I wonder if the 0.95 should be outright removed.

src/arkode/arkode_lsrkstep.c

Steven-Roberts · 2024-09-19T22:14:10Z

src/arkode/arkode_lsrkstep_impl.h

+  int domeigfreq; /* indicates dom_eig update after domeigfreq successful steps*/
+
+  /* Flags */
+  sunbooleantype isextDomEig; /* flag indicating user provided dom_eig */


This looks equivalent to checking if extDomEig != NULL in which case it could be removed. Maybe having both will support future extensions

That is true. It is just an unused variable for the time being. Should I remove it until we get to that point?

examples/arkode/CXX_parallel/ark_heat2D_lsrk_p.cpp

maggul added the pkg-ARKODE label Jul 12, 2024

drreynolds and others added 28 commits July 12, 2024 11:02

Added placeholder files for new low-storage Runge--Kutta time steppin…

73b1e54

…g module

Ran formatter

6129d28

A dummy changes to test pushing.

5093dde

An sts example has been created

d91e292

A new STS example

a34da9c

A dummy change!

1de727a

STS example created

db1496f

Some progress!

26d00ac

Some progress!

1834543

Some progress

fb99186

Some progress

70b423f

some progress

8dc07d4

some progress

dfa6439

some progress

d5a72d2

some progress

f35f3f0

some progress

0bb571d

some progress

39833f5

some progress

c3d9825

Updated Examples

3ae5e9f

some progress

6396d58

some progress

74f5640

some progress

7ba800f

revert

a9c2e89

some progress

9b4f6d6

some progress

d201098

some progress

ba4f81f

some progress

fecfd2c

some progress

c62e634

drreynolds added 7 commits September 17, 2024 09:35

Minor cleanup

2c90942

Renamed 'explicit' to 'explicitSTS'

c688249

Fixed conversion 'error'

a97df27

formatting

9a3c241

Fixed conversion 'error'

d54bccc

Fixed unused variable 'error'

8abb3ff

Fixed unused variable, shadow, and reorder 'errors'

5c16d7f

balos1 requested changes Sep 17, 2024

View reviewed changes

drreynolds and others added 7 commits September 17, 2024 12:14

Fixed newly-introduced compilation error; added analytical solution f…

03536f5

…or sod shock problem

Fixed unused variable 'error'

93c9f79

formatting

5544e8d

formatting

85ba7cb

revision

08e764d

revision

e28bdfd

formatting

3b35ba1

Steven-Roberts requested changes Sep 17, 2024

View reviewed changes

maggul and others added 4 commits September 18, 2024 12:49

precision

ac99a77

revision

8a39c1a

Applied some suggestions from code review

dc7f680

Made recommended changes from code review

65b62df

Steven-Roberts reviewed Sep 19, 2024

View reviewed changes

maggul and others added 7 commits September 19, 2024 13:59

revision

22b75fe

Moved 'using namespace std' to .cpp file

1187945

removed use of SUNR* math functions

9bfeb7c

merged

8850586

formatting

bf8c1dd

nfe

9fc1e3a

Merge branch 'feature/arkode-sts' of github.com:LLNL/sundials into fe…

90c4ea6

…ature/arkode-sts

Steven-Roberts reviewed Sep 19, 2024

View reviewed changes

revision

7cd161f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/arkode sts #541

Feature/arkode sts #541

maggul commented Jul 12, 2024

drreynolds commented Jul 12, 2024

balos1 left a comment

balos1 Sep 17, 2024

balos1 Sep 17, 2024

maggul Sep 19, 2024

Steven-Roberts left a comment

Steven-Roberts Sep 17, 2024

Steven-Roberts Sep 17, 2024

Steven-Roberts Sep 19, 2024

Steven-Roberts Sep 19, 2024

Steven-Roberts Sep 19, 2024

Steven-Roberts Sep 19, 2024

Steven-Roberts Sep 19, 2024 •

edited

Loading

Steven-Roberts Sep 19, 2024

Steven-Roberts left a comment

Steven-Roberts Sep 19, 2024

maggul Sep 19, 2024

Steven-Roberts Sep 19, 2024 •

edited

Loading

Steven-Roberts Sep 19, 2024

maggul Sep 19, 2024


		When running LSRKStep with either the RKC or RKL methods, the user must supply a dominant eigenvalue estimation function of type :c:type:`ARKDomEigFn`:

		.. c:type:: int (ARKDomEigFn)(sunrealtype t, N_Vector y, sunrealtype* lambdaR, sunrealtype* lambdaI, void* user_data)

		const sunrealtype onep54 = SUN_RCONST(1.54), c13 = SUN_RCONST(13.0),
		p8 = SUN_RCONST(0.8), p4 = SUN_RCONST(0.4);

Feature/arkode sts #541

Are you sure you want to change the base?

Feature/arkode sts #541

Conversation

maggul commented Jul 12, 2024

drreynolds commented Jul 12, 2024

balos1 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

Steven-Roberts 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

Steven-Roberts Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Steven-Roberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Steven-Roberts Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Steven-Roberts Sep 19, 2024 •

edited

Loading

Steven-Roberts Sep 19, 2024 •

edited

Loading