Repository navigation
fix(biogeophys): Correct diagnostic array initialization for exact restarts - #4172
slevis-lmwg merged 6 commits into
Conversation
b149868 to
f6919a7
Compare
|
@johnpaulalex I think this PR blocks #4171, so I added the corresponding label. Pls correct me if I'm wrong. |
johnpaulalex
added
closed: wontfix
|
Thanks @slevis-lmwg ! True, but also note that this issue itself depends on NGEET/fates#1596. Should I mark 1596 as a blocker? And then does 1596 get submitted, then cesm is pointed to the updated FATES version, then this PR can be submitted, and finally 4171? |
|
That sounds correct. |
slevis-lmwg
added
the
blocked: dependency
ekluzek
left a comment
ekluzek
left a comment
There was a problem hiding this comment.
Interesting, this allows us to not have to add the diagnostic variables to restarts which is good. I'm also impressed that the AI figured that out.
It has the cost of resetting diagnostic variables to spval each time step. I doubt it has to be redone every time step though. The pattern of where the spval need to be shouldn't change for most of these variables. Only if it's a transient simulation in some cases, or if they are bound to something like whether snow is on the ground or not. And I don't think either applies here. At least in general there are a couple cases that I spell out in comments where it does need to be each timestep. Others I think can probably be done only once.
I suggest maybe changing the name of TimeStepInit to SetValue with spval passed in (matches biogeochem code). And I think spinning off some of these things into tiny subroutines might be appropriate.
So marking this with requesting changes, but also happy to discuss if you disagree.
But, there must be something going on where the pattern can be wrong on restart. From the earlier PR #4142 we can surmise that it's correct when written to the restart file and when read in from restart. But, if not read on restart, the pattern must not be correct and it causes problems. So maybe this just has to be done at initialization?
@johnpaulalex could you try (or did you try) having this done just at initialization and not each timestep? Although that should be taken care of by the normal initial setting of things to spval because they are output to history files. But, maybe it needs to be reset after that point and before used? What if TimeStepInit was only done once? So you added a flag to see if it's already been set and it only allows it to be set the first time?
|
@johnpaulalex I reset the PR to go to b4b-dev as we talked about. It looks like you don't need to rebase the branch, but you should at least pull in the latest b4b-dev. And b4b-dev and master is going to be reconciled early next week, so you could wait for that. |
|
yeah good point @ekluzek . ~3 vars truly needed to be reset every time step. The rest are now done at init time. And for the transisent case, that could apparently happen for all those other vars? See the second new commit for that, ResetDeactivatedDiagnostics in dynPatchStateUpdaterMod.F90. I also switched the name to SetValue and added helper subroutines. |
Ah I can't set labels on the FATES repo. @ekluzek fyi, not sure if I should have permissions or ask you to do it. |
ack, I just rebased to b4b-dev @ekluzek |
|
@slevis-lmwg @ekluzek @glemieux I have updated description of CTSM PRs 4171, 4172, and FATES 1596 to establish their relationship to fixing CTSM issues 3660, 3661, and 4142. TL;DR the PRs need to be submitted in 1596->4172->4171 order. PR 4172 resolves Issues 3661 and 4142, and PR 4171 resolves issue 3660. |
There was a problem hiding this comment.
My initial testing
- Uncomment the fields that I commented out in #4136 for the all_outputs test to pass:
./create_test ERP_P64x2_D_Ld3.f10_f10_mt232.IHistClm60BgcCropCrujra.derecho_gnu.clm-default--clm-all_outputs -c /glade/campaign/cgd/tss/ctsm_baselines/ctsm5.4.058: FAIL - Revert to the commented out code (git stash): FAIL
- Update branch with the latest b4b-dev: FAIL
- ctsm5.4.058-39-g703c29e42 which was the lastest b4b-dev at the time: PASS
- Once the above cases work, run with fates code that includes NGEET/fates#1596: WAIT
39ae88f to
af199ce
Compare
af199ce to
39ae88f
Compare
|
@slevis-lmwg ok rereading my AI chat history, I thought the system test passed, but it didn't. I now have it passing, and have two decisions to make:
More on PREV_DAYL per Claude: PREV_DAYL is the previous time step's daylength, used by phenology and not stored on the restart file, so at startup CTSM rebuilds it from the calendar one step back using the orbital parameters of the current year. When a run restarts exactly at Jan 1 00:00 in variable-year orbital mode, that reconstruction uses the new year's obliquity and eccentricity while the continuous run computed the same value with the old year's, giving about a 0.1-second mismatch on that one step and a failed exact-restart comparison. |
… at year boundary PREV_DAYL is the previous timestep's daylength, reconstructed at startup from calendar declination. When restarting exactly at Jan 1 00:00 in variable-year orbital mode (e.g. IHist compsets), the restart run computed the previous step's declination using the new year's orbital parameters, while the continuous run used the old year's parameters, causing a small discrepancy on the first timestep. Store previous time step orbital parameters in clm_varorb module variables to compute caldaym1 declination consistently on restart.
… on restart Snow thermal conductivity diagnostics (SNO_TK, bw, thk) are computed in SoilThermProp before new snow layers are created or divided later in the timestep, causing uninitialized or stale layer entries to leak into history output. Add InitSnowThermPropDiagnostics to initialize bw and thk to spval across snow layers, passing clump sections explicitly to prevent corruption in multi-clump threaded runs.
…xact restart Exact-restart divergence occurs when history fields persist state across timesteps on patches or conditions that stop being updated, because these fields are not stored in restart files: 1. Patches leaving the exposed-vegetation filter in CanopyFluxes stopped having their diagnostics updated, retaining stale memory in continuous runs while being initialized to spval on restart. Add CanopyFluxesResetDiagnostics to reset these fields when frac_veg_nosno transitions from 1 to 0. 2. Restrict FrictionVelocity%SetValues to reset only the 10 diagnostic arrays calculated exclusively within CanopyFluxes (rah1, rah2, raw1, raw2, ustar, um, uaf, taf, qaf, vpd), deliberately preserving prognostic restart variables (obu) and general boundary-layer fields computed across all patch types. 3. Add SoilStateType%SetValues to reset root and soil conductance diagnostics (KROOT, KSOIL) when patches leave the exposed-vegetation filter. 4. Set RRESIS (root resistance) to 0._r8 in SoilMoistStressMod when soil layers are frozen or dry; previously frozen layers retained pre-freeze values, causing divergence against winter restarts.
Exact-restart divergence occurs when diagnostic history fields retain stale values across timesteps because they are not stored in restart files: 1. In VOCEmissionMod, reset MEGAN activity factor diagnostics (gamma, gammaP, gammaT, alpha, topt, etc.) to spval via an explicit else branch when gamma is out of range (imeg==1). Previously, out-of-range patches retained the prior timestep's values, whereas restarted runs initialized them to spval. 2. In NutrientCompetitionFlexibleCNMod, set actual_storage_leafcn to spval when flexible CN storage is inactive or non-positive (leafc_storage_patch <= 0 or leafn_storage_patch <= 0). Previously, zero-storage patches retained the last non-zero ratio indefinitely.
… disabled by ESCOMP#3661 Uncomment all 35 history fields across VOCEmissionMod, FrictionVelocityMod, SoilStateType, WaterDiagnosticBulkType, EnergyFluxType, and NutrientCompetitionFlexibleCNMod that were temporarily disabled in ESCOMP#3661 pending exact restart resolution.
|
@slevis-lmwg Here's the diff against squashed clean commits, I think it's much easier to review than the original commits + deltas. I reran against derecho and it passed[1] Sorry again for not having my ducks in a row in the initial submit. [1] Path: |
|
I'm testing in the same order that I tested before (note that the testname has the grid updated to t233):
I'm afraid this does not work for me, yet. I'm working here in case you want to see what's different between mine and yours: Also, I could not find your test that you said had passed. |
bac78c6 to
12a51bb
Compare
|
Hi @slevis-lmwg, so sorry, miscommunication. I had created the other branch for easier review (6 commits), but I wasn't sure if you wanted me to stack the equivalent delta on top of this PR's branch, so I hadn't touched it. So no surprise this PR's branch failed again. Now: I've replaced this PR's branch with those 6 commits, including a b4b-dev sync and uncommenting all the history fields. It passed the newer mt233 test [2]. I also fixed the system path in my prior comment (which was recording a passing run , on the older mt232, against the 6-commit branch minus the b4b-dev sync) If for whatever reason you wanted to look at the original 12 commits for this PR (plus a newly added commit to make them exactly match the 6-commit branch), they are stowed in my backup/4124-3661-correct-restart-inits branch. John [2] /glade/derecho/scratch/jpalex/ERP_P64x2_D_Ld3.f10_f10_mt233.IHistClm60BgcCropCrujra.derecho_gnu.clm-default--clm-all_outputs.t4124_mt233/ |
|
@johnpaulalex thanks and sorry for not catching on to your earlier comment that I needed to test on the alternate branch. In any case, now the test passed, as you expected :-) Next:
|
|
I have now also looked at the PREV_DAYL updates that you discussed here. If aux_clm passes, I will consider these updates good for b4b-dev. |
|
I answered my own question about the "missing" test from |
|
Sounds great! Thanks for your patience @slevis-lmwg . To be sure we're on the same page, this is my understanding:
In general should I be running izumi system tests in parallel with derecho? (same aux_clm suite if so?) Or is that something the approver is going to do anyway? |
Erik's review was addressed and the comments marked Resolved.
I consider this ready to merge.
If you have access to izumi, then you could run aux_clm on izumi at the same time as on derecho. Once in a while izumi will catch something that derecho did not. The PR merger runs those tests unless the PR author runs them and indicates that things worked. |
github-project-automation
Bot
moved this from In Progress
to Done
in LMWG: Sprint Planning Board
github-project-automation
Bot
moved this from In progress - b4b-dev
to Done (non release/external)
in CTSM: Upcoming tags
Description of changes
This change fixes exact restart (ERP) failures when
hist_all_fields = .true.by ensuring diagnostic variables acrossBiogeophysPreFluxCalcsMod,CanopyFluxesMod,FrictionVelocityMod,SoilHydrologyMod,SoilMoistStressMod,SoilStateType, andSoilTemperatureModare properly initialized tospvaleach timestep on inactive layers or patches. Without explicit per-timestep initialization, uninitialized array space on inactive points retained memory across timestep boundaries, causing state discrepancies when comparing base vs. restart history output.Linked issues addressed, if any:
FatesColdAllVarsrestart failure)Dependencies / Blockers:
Description of generative AI usage:
Answer Changes & Scientific Impact
User Interface & Namelist Changes
Testing planned or performed, if any:
CTSM / CESM baseline hash-tag: 7409d32
PR branch hash-tag: 3a0dc8a
Requirements before merge: