Conversation
TestSubcycleStepping/test/subcycle/carpetxregrid-regrid_error.it000000.x.tsv
Show resolved
Hide resolved
TestSubcycling/test/subcycle/testsubcycling-iteration.it000008.z.tsv
Outdated
Show resolved
Hide resolved
rhaas80
left a comment
rhaas80
left a comment
There was a problem hiding this comment.
no further objections from me. But I have not looked deeply into the parts that Erik already commented on.
| CarpetX::out_tsv_vars = " | ||
| TestSubcycling::iteration | ||
| CarpetXRegrid::regrid_error | ||
| " |
There was a problem hiding this comment.
do we need .x., .y. and .z. output for all variables? Or would .y. be sufficient alone to guard against regressions?
There was a problem hiding this comment.
I would still claim that the files are pretty small ;)
There was a problem hiding this comment.
Since the canary
CCTK_REAL canary = 100 + 10 * cctk_iteration + 1 * cctk_level;
iteration(pt.I) = canary;
is actually constant and we do not do anything that varies is space, two suggestions:
- can we get away with a single grid point?
- output only one grid point
There was a problem hiding this comment.
no. we need mesh refinement to test the subcycle stepping. we can't do mesh refinement with only one grid point. the fact the it's constant making it easier for us to tell if the subcycle stepping is correct or not
eschnett
left a comment
eschnett
left a comment
There was a problem hiding this comment.
I made a few suggestions. Please look at them, then merge.
CarpetX/src/schedule.cxx
Outdated
| cctkGH->cctk_delta_time = dtfac * mindx; | ||
| cctkGH->cctk_delta_time = | ||
| dtfac * | ||
| (ghext->use_subcycling() ? get_coarse_mindx() : get_finest_mindx()); |
There was a problem hiding this comment.
it's been a while. Do we set cctkGH->cctk_timefac and cctkGH->cctk_levfac (probably needs to happen in enter/leave_level_mode or so.
There was a problem hiding this comment.
cctk_levfac are the same between subcyeling and non-subcycling. Where does cctk_timefac used?
There was a problem hiding this comment.
cctk_timefac is used in CCTK_DELTA_TIME:
src/include/cctk_core.h:#define CCTK_DELTA_TIME (cctk_delta_time/cctk_timefac)
we do actually update it:
CarpetX/src/schedule.cxx: cctkGH->cctk_timefac = 1; // no subcycling
CarpetX/src/schedule.cxx: cctkGH->cctk_timefac = sourceGH->cctk_timefac;
CarpetX/src/schedule.cxx: cctkGH->cctk_timefac = ghext->use_subcycling ? (1 << min_level) : 1;
Are you using CCTK_DELTA_TIME in the ODESolvers? If so that is already a test for the value being set and being set correctly.
There was a problem hiding this comment.
Liwei says out-of-band that CCTK_DELTA_TIME is used.
| } | ||
| min_active_level = level; | ||
| } | ||
| assert(min_active_level != -1); |
There was a problem hiding this comment.
Erik had pointed out (correctly out one assumes) that once one has found the coarsest level that steps forward this iteration then all levels finer than that one must also step forward, which could be added as an assert() on the logic of the code here.
There was a problem hiding this comment.
though, looking at the // Find end of batch comment below, those that have a different timestep size would not be part of the batch, but of a different batch that will step this iteration.
There was a problem hiding this comment.
should I do anything about it? or it's fine for now
There was a problem hiding this comment.
I think it is fine. Erik's comment would then apply to the min_level variable. I will add a sanity check.
| active_levels = make_optional<active_levels_t>(min_level, max_level); | ||
|
|
||
| if ((!restrict_during_sync) && | ||
| (ghext->patchdata.at(0).leveldata.at(max_level - 1).delta_iteration == |
There was a problem hiding this comment.
ugly (b/c of the long series of . operations) and probably dicey in case patch 0 does not actually have max_level levels (that is if Lucas ever decides to make patch 0 to be not the Cartesian one or if we actually have mesh refinement in the curvilinear patches). What does that condition actually try to check? Bunch up the same levels we just stepped forward? it's not the set of levels we have decided have now caught up with each other. Couldn't active_levels contain levels for which this is not true? I am confused by this logic.
There was a problem hiding this comment.
We are trying to do restriction (and POSTRESTRICT) only when the finest level finish the current time stepping.
For example,
----- ----- -----
4/4
4/4 -----
3/4
4/4 ----- -----
2/4
2/4 -----
1/4
----- ----- -----
we don't want to call Restrict (and PostRestrict) when level_iteration=4/4 at level 1, but at level 2 (the finest level). The reset of active_levels one line above make it contains all the level (all of them have level_iteration=4/4).
We are using delta_iteration to decide if we are stepping the finest level or not.
| CarpetX::out_tsv_vars = " | ||
| TestSubcycling::iteration | ||
| CarpetXRegrid::regrid_error | ||
| " |
There was a problem hiding this comment.
Since the canary
CCTK_REAL canary = 100 + 10 * cctk_iteration + 1 * cctk_level;
iteration(pt.I) = canary;
is actually constant and we do not do anything that varies is space, two suggestions:
- can we get away with a single grid point?
- output only one grid point
4 participants
Summary Refactored
driver.cxxandschedule.cxxto enable subcycling support in CarpetX.Change Log
Regression Test: Added a WaveToyX test (3 refinement levels) to ensure backward compatibility.
Parameter Scope: Promoted
use_subcycling_wipto RESTRICTED scope for external thorn usage.Driver Logic: Enabled subcycling during regridding (provided levels align) in
driver.cxx.Schedule Logic: Updated
schedule.cxxto support the new subcycling flow.Unit Test: Added a new test case specifically for subcycle time stepping.