-
Notifications
You must be signed in to change notification settings - Fork 123
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/arkode sts #541
base: develop
Are you sure you want to change the base?
Feature/arkode sts #541
Conversation
Unfortunately, the rebase to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I have made it through the documentation.
============================================ | ||
|
||
The LSRKStep time-stepping module in ARKODE supports a variety of so-called | ||
"low-storage" Runge--Kutta methods. This category includes traditional explicit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you cite some references for these different types of low-storage methods?
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to use this even in the real valued problems, suncomplextype
might not be appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
#include <sundials/sundials_core.h> // Provides core SUNDIALS types |
+-----------------------------------------------+------------------------------------------------------------+ | ||
| :index:`ARKODE_LSRK_RKL` | 2nd order Runge-Kutta-Legendre (RKL) method. | | ||
+-----------------------------------------------+------------------------------------------------------------+ | ||
| :index:`ARKODE_LSRK_SSPs_2` | Optimal 2nd order s-stage SSP RK method. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this value be all caps?
include/arkode/arkode_lsrkstep.h
Outdated
{ | ||
ARKODE_LSRK_RKC = 1, /* ensure enum is int */ | ||
ARKODE_LSRK_RKL = 2, | ||
ARKODE_LSRK_RKG = 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine since it will be undocumented presumably. I don't see why the particular value matters.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
{ | ||
case ARKODE_LSRK_RKC_2: | ||
ark_mem->step = lsrkStep_TakeStepRKC; | ||
step_mem->LSRKmethod = ARKODE_LSRK_RKC_2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be brought out of the switch to step_mem->LSRKmethod = method
. These are common sources of copy paste errors.
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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); | |
} |
step_mem->constJac = SUNTRUE; | ||
step_mem->domeigfreq = 1; | ||
} | ||
else { step_mem->domeigfreq = nsteps; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this branch set step_mem->constJac = SUNFALSE
?
/* LSRK method storage and parameters */ | ||
// N_Vector temp1; /* Temp vector storage */ | ||
// N_Vector temp2; /* Temp vector storage */ | ||
N_Vector* Fe; /* RHS vector storage */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
const sunrealtype onep54 = SUN_RCONST(1.54), c13 = SUN_RCONST(13.0), | ||
p8 = SUN_RCONST(0.8), p4 = SUN_RCONST(0.4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A citation on where the come from would be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
} | ||
if (ss == step_mem->stagemaxlimit) | ||
{ | ||
hmax = SUN_RCONST(0.95) * (SUNSQR(ss) + ss - 2) / (2.0 * step_mem->sprad); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea: use hadapt_mem->safety
instead of 0.95
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, sprad
already has the domeigsfty
applied, so I wonder if the 0.95
should be outright removed.
int domeigfreq; /* indicates dom_eig update after domeigfreq successful steps*/ | ||
|
||
/* Flags */ | ||
sunbooleantype isextDomEig; /* flag indicating user provided dom_eig */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks equivalent to checking if extDomEig != NULL
in which case it could be removed. Maybe having both will support future extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true. It is just an unused variable for the time being. Should I remove it until we get to that point?
This is a draft PR for feedback on new STS module.