MRI-adaptivity #564
MRI-adaptivity #564
Conversation
drreynolds
requested review from
balos1,
gardner48 and
cswoodward
as code owners
|
I believe that I've finished addressing all PR comments. Aside from Jenkins, this is passing all CI tests, so once I VPN in to identify/resolve those (likely changes in some |
drreynolds
dismissed
gardner48’s stale review
dismissing previous review, since GitHub can no longer show me any remaining requested changes.
src/arkode/arkode_mristep.c
Outdated
| /* Signal that any pre-existing RHS vector is no longer current, since it | ||
| has a stale forcing function */ | ||
| ark_mem->fn_is_current = SUNFALSE; |
There was a problem hiding this comment.
I think this is still needed. Since the forcing changed the stored fn is now invalid.
Co-authored-by: David Gardner <gardner48@llnl.gov>
Co-authored-by: David Gardner <gardner48@llnl.gov>
Co-authored-by: David Gardner <gardner48@llnl.gov>
There was a problem hiding this comment.
@gardner48: I accepted your more obvious suggestions, but I have questions about others (see comments below).
src/arkode/arkode_mristep.c
Outdated
| /* if the method had a stiffly-accurate internal stage, use the | ||
| already-computed RHS vectors */ | ||
| sa_stage = -1; | ||
| for (i = 1; i < step_mem->stages; i++) |
There was a problem hiding this comment.
I don't believe so. Since the embedding stage is stored in the last row of the tables, it is possible that the "stiffly accurate" stage is the next-to-last one stored. I'll test whether I can shorten this loop to check.
| } | ||
| ark_mem->fn_is_current = SUNTRUE; |
There was a problem hiding this comment.
I'm not sure I fully understand your concern here, but I am confident that this suggestion is not the way to proceed.
fn is only ever used with the Hermite interpolation module, when temporal root-finding is enabled, or by steppers that wish to leverage its data directly (e.g., ERKStep); notably, fn is never directly used by MRIStep. Thus, depending on the choice of interpolation module and temporal rootfinding, it is possible that fn may be NULL, so we should not copy data into it directly.
To ensure that I can understand your concern, let me explain what I think you're saying. Since the TakeStep routines do not call FullRHS directly in ARK_FULLRHS_START mode, then ark_mem->fn may not be current at the start of the step. Thus, anything inside the step that assumes fn is up-to-date will be accurate? Given that we already disallow users from calling ARKodeGetDky from inside a current step, then I suppose the only real concern would be if the Hermite interpolation module is used for higher-order implicit predictors? Are there other contexts where you think that fn needs to be current from inside the step calculation?
Separately, if fn may not be current at the start of the step, are you concerned that it might not be updated correctly at the end of the step (i.e., before ARKodeGetDky and temporal root-finding are used)?
| potentially get inner dsm on all non-embedding stages */ | ||
| /* Reset the inner stepper on the first stage within all but the | ||
| first stage group due to "stage-restart" structure */ | ||
| if ((stage > 1) && (is == 0)) |
There was a problem hiding this comment.
I believe so, yes.
I think that this PR is now ready for full review.
Note: I removed all of the changesets that I'd pulled from @Steven-Roberts's PR #547 to simplify the review process for this PR.