-
Notifications
You must be signed in to change notification settings - Fork 125
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
arkode-fullrhs #293
arkode-fullrhs #293
Conversation
…ing module (and in turn, the MRIStepInnerStepper module).
…le (output files now differ due to reduced numbers of RHS calls, particularly for MRI).
doc/arkode/guide/source/Usage/MRIStep_c_interface/Custom_Inner_Stepper/Description.rst
Outdated
Show resolved
Hide resolved
doc/arkode/guide/source/Usage/MRIStep_c_interface/Custom_Inner_Stepper/Description.rst
Show resolved
Hide resolved
MSG_ARK_RHSFUNC_FAILED); | ||
return(ARK_RHSFUNC_FAIL); | ||
} | ||
} |
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.
I think this could be removed by adding || ark_mem->h == ZERO
to if (ark_mem->call_fullrhs_start || ark_mem->call_fullrhs_end)
in arkInitialSetup
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.
@gardner48 Are you referring to this entire change block?
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.
Yes
@@ -766,6 +766,18 @@ int erkStep_TakeStep(void* arkode_mem, realtype *dsmPtr, int *nflagPtr) | |||
ark_mem->nst, ark_mem->h, ark_mem->tcur); | |||
#endif | |||
|
|||
/* if fullrhs has not been called, fill in F[0] */ | |||
if (!ark_mem->call_fullrhs_start) { |
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 should not be possible, since nothing should set call_fullrhs_*
to false and erkStep_Init
sets both to true.
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.
It seems like it may be safer to handle this even though it is not currently possible to reach it. If it must always be a broken state to reach this, then an error should be thrown if it is reached.
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.
It might be better to throw an error so we don't unintentionally end up with an extra RHS evaluation at the start of each step.
@@ -1462,6 +1465,30 @@ int mriStep_TakeStep(void* arkode_mem, realtype *dsmPtr, int *nflagPtr) | |||
ark_mem->nst, ark_mem->h, ark_mem->tcur); | |||
#endif | |||
|
|||
/* if fullrhs has not been called, fill in Fse[0] and Fsi[0] as applicable */ | |||
if (!ark_mem->call_fullrhs_start) { |
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.
Here call_fullrhs_start
is at the start of each step but I think this could similarly be accomplished by
- Filling
fse[0]
andfsi[0]
in the stepper init - If FullRHS is called in
arkInitialSetup
it just copies formfse[0]
andfsi[0]
and adds on the fast time scale contribution - At the end of a step fill
fse[0]
andfsi[0]
for the next step unlesscall_fullrhs_end
is true in which case the FullRHS call inarkCompleteStep
will fill these
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.
@gardner48 What is the advantage of your proposal?
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.
I think this would make the code more consistent. call_fullrhs_start
is really more like call the full RHS at initialization and then this call (like in ERKStep) never actually happens and the full RHS is actually updated at the end of the step.
Co-authored-by: David Gardner <[email protected]>
Closing this PR (similar updates are included in an upcoming PR by @gardner48) |
This PR makes the necessary changes so that
fullrhs
is an optional routine for both an ARKODE time-stepping module, and for anMRIInnerStepper
module. To test this, the PR also converts many ARKODE examples to request the Lagrange interpolation module.The PR is not quite complete, but I'm submitting it now for 2 reasons:
.out
files for the now-changed results.In addition to missing the new
.out
files, this PR also needs updates to both the ARKODE documentation andCHANGELOG.md
.