ctsm5.3.077: Removing non doalb call of wrap_canopy_radiation for fates

[rgknox]

Description of changes

This reverts a previous change that enabled a call to fates' wrap_canopy_radiation and zenith angle updates when the doalb flag was false. The original implementation was (apparently) needed for elm-fates to enable branch type restart runs, but appears to be unnecessary for clm-fates. The previous code was causing problems for cam-fates coupled runs.

Specific notes

Contributors other than yourself, if any: @mvdebolskiy performed tests, @ekluzek and @mvertens provided consultation

CTSM Issues Fixed (include github issue #):
Fixes: #3043

Are answers expected to change (and if so in what way)? no expected changes

Any User Interface Changes (namelist or namelist defaults changes)?

Does this create a need to change or add documentation? Did you do so?

Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on derecho for intel/gnu and izumi for intel/gnu/nag/nvhpc is the standard for tags on master)

so far: ERI_Ld10.f45_f45_mg37.I2000Clm50FatesCru.derecho_gnu.clm-FatesColdTwoStream and ERS_Ld3.f19_g17.I2000Clm50FatesCru.derecho_gnu.clm-FatesColdTwoStream

NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).

[mvdebolskiy]

I am testing this in coupled cases.
Also, I want to add a testmod for fates that includes DATM namelist changes same as clm6cam7LndTuningMode. But that can be a separate PR.

[mvdebolskiy]

Also, there is a need to not allow WARNING in the BalanceCheckMod when fates is on, since the solver there has a different tolerance.
Also in clm-fates interface you are passing non-downscaled short-wave, is there specific reason for this?

[rgknox]

@mvdebolskiy , non-downscaled shortwave could just be a a legacy issue, ie we've been using it before a downscale version became available, but I'm just guessing. It has been that way since fates was clm-ed. I suppose there is no reason to not use the most downscaled version possible, but would need to look into how the downscaling is done and what is the actual change of scales (or what subgrid variability is this covering) before I could weigh-in .

[mvdebolskiy]

The downscaling is just for hillslope hydrology, in general case it equals to greedcell one, so if there is a need to have fates compatible with it, than the column level, downscaled sw should be used, same way as column level coszen.

[ekluzek]

Yes, @mvdebolskiy is right, you should use downscaled...

The downscaling is just for hillslope hydrology, in general case it equals to gridcell one, so if there is a need to have fates compatible with it, than the column level, downscaled sw should be used, same way as column level coszen.

In columns where that don't have downscaling, they'll be identical, but you'll want it for those cases that do use it. So you should just always use the downscaled version.

Downscaling is also done for glaciers -- which doesn't apply to FATES. But, the point is -- you'll want it for any cases where it has been done.

[ekluzek]

I am testing this in coupled cases. Also, I want to add a testmod for fates that includes DATM namelist changes same as clm6cam7LndTuningMode. But that can be a separate PR.

I'd actually like to get this new testcase installed in this PR. Partially, so that we can show that the new test breaks without the change, and it passes with it. This should be easy enough to do. I don't think adding this test will slow this effort down enough that it can't be done now.

[rgknox]

@ekluzek and @mvdebolskiy can you take a look at the new tests and give feedback corrections? this is not my wheelhouse

[mvdebolskiy]

@rgknox they look fine, one thing that might be useful is to make one to have two-tream. You can make it full fates, actually, not just no-comp fixed-biogeo, since the late flags should not matter for the purpose of this tests.

[mvdebolskiy]

Removing the call for update and normalized radiation breaks hybrid/startup with finidat runs.
I added the call back, but restricted it only to fates nocomp with twostream and first timestep on the startup run here.
(just remove the diagnostic writeout before wrap_sunfrac).

[rgknox]

Ok, thanks for noting that @mvdebolskiy

[rgknox]

@mvdebolskiy could you post the error for the break? I'm wondering if other fixes that have been implemented, such as the cross-referenced PR above may address the problem

[rgknox]

@mvdebolskiy your a legend, I'm getting passes in the following test:

ERI_D_Ld9.f45_f45_mg37.I2000Clm60Fates.derecho_gnu.clm-FatesColdCamLndTuningMode

Should we get this into the integration pipeline soon?

[mvdebolskiy]

I guess so, can you try the same test but with SP?

[rgknox]

I think this PR is close to ready. Recent changes:

1. I merge it up to master
2. expanded test coverage per recommendations of SE team about 3 weeks ago
3. I tested with the following FATES-side PR: radiation refactors - rad_error and rad profile diagnostics NGEET/fates#1397 (ERI_D_Ld9.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhenCamLndTuningMode.20250915_093212_ymt1x8), all PASS

[rgknox]

Below is the test fail list as of f427877. Note that most of these are either expected, or simply missing bases since the tests are new. The only test fail that I see is the restart compare on the new ERI test (ERI_D_Ld9.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhenCamLndTuningMode.C.0921-123413de_gnu), and it is just relegated to diffs on the fates radiation error diagnostics:

RMS FATES_NIR_RAD_ERROR 5.3545E+33 NORMALIZED 1.5978E-02

Although, there is a good chance this will be fixed in FATES 1397

Either way, we need to move forward, I added an expected fail for the test. I will note this in an issue and we can work on it if 1397 does not fix the problem. The good part is that judging by the numbers, this does not seem to be impacting scientifically meaningful results in the restart.

921-123413de_gnu: 14 tests
    FAIL ERI_D_Ld9.f45_f45_mg37.I2000Clm60Fates.derecho_gnu.clm-FatesColdCamLndTuningMode NLCOMP
    FAIL ERI_D_Ld9.f45_f45_mg37.I2000Clm60Fates.derecho_gnu.clm-FatesColdCamLndTuningMode BASELINE fates-sci.1.87.2_api.41.0.0-ctsm5.3.075-v2: ERROR BFAIL baseline directory '/glade/campaign/cgd/tss/ctsm_baselines/fates-sci.1.87.2_api.41.0.0-ctsm5.3.075-v2/ERI_D_Ld9.f45_f45_mg37.I2000Clm60Fates.derecho_gnu.clm-FatesColdCamLndTuningMode' does not exist
    FAIL ERI_D_Ld9.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhenCamLndTuningMode NLCOMP
    FAIL ERI_D_Ld9.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhenCamLndTuningMode COMPARE_base_rest (EXPECTED FAILURE)
    FAIL ERI_D_Ld9.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhenCamLndTuningMode BASELINE fates-sci.1.87.2_api.41.0.0-ctsm5.3.075-v2: ERROR BFAIL baseline directory '/glade/campaign/cgd/tss/ctsm_baselines/fates-sci.1.87.2_api.41.0.0-ctsm5.3.075-v2/ERI_D_Ld9.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhenCamLndTuningMode' does not exist
    FAIL ERS_D_Ld5.f45_f45_mg37.I2000Clm60Fates.derecho_gnu.clm-FatesColdCamLndTuningMode NLCOMP
    FAIL ERS_D_Ld5.f45_f45_mg37.I2000Clm60Fates.derecho_gnu.clm-FatesColdCamLndTuningMode BASELINE fates-sci.1.87.2_api.41.0.0-ctsm5.3.075-v2: ERROR BFAIL baseline directory '/glade/campaign/cgd/tss/ctsm_baselines/fates-sci.1.87.2_api.41.0.0-ctsm5.3.075-v2/ERS_D_Ld5.f45_f45_mg37.I2000Clm60Fates.derecho_gnu.clm-FatesColdCamLndTuningMode' does not exist
    FAIL PEM_D_Ld20.5x5_amazon.I2000Clm50FatesRs.derecho_gnu.clm-FatesColdSeedDisp COMPARE_base_modpes (EXPECTED FAILURE)
    FAIL SMS_D_Ld3.f09_g17.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhen_prescribed RUN time=74 (EXPECTED FAILURE)
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_gnu.clm-FatesPRISM--clm-NEON-FATES-YELL SHAREDLIB_BUILD time=124 (UNEXPECTED: expected FAIL)
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_gnu.clm-FatesPRISM--clm-NEON-FATES-YELL RUN time=101 (UNEXPECTED: expected FAIL)

 
0921-123413de_int: 40 tests
    FAIL ERI_D_Ld20.f10_f10_mg37.I2000Clm50Fates.derecho_intel.clm-FatesCold BASELINE fates-sci.1.87.2_api.41.0.0-ctsm5.3.075-v2: DIFF
    FAIL ERI_Ld60.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-Fates BASELINE fates-sci.1.87.2_api.41.0.0-ctsm5.3.075-v2: DIFF
    FAIL ERP_P256x2_Ld30.f45_f45_mg37.I2000Clm60FatesRs.derecho_intel.clm-mimicsFatesCold RUN time=43 (EXPECTED FAILURE)
    PEND ERP_P256x2_Ld30.f45_f45_mg37.I2000Clm60FatesRs.derecho_intel.clm-mimicsFatesCold COMPARE_base_rest
    PASS ERS_Ld60.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdST3 RUN time=118 (UNEXPECTED: expected FAIL)
    FAIL PVT_Lm3.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesLUPFT RUN time=1590 (EXPECTED FAILURE)
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_intel.clm-FatesFireLightningPopDens--clm-NEON-FATES-NIWO SHAREDLIB_BUILD time=104 (UNEXPECTED: expected FAIL)
    PASS SMS_Ld10_D_Mmpi-serial.CLM_USRDAT.I1PtClm60Fates.derecho_intel.clm-FatesFireLightningPopDens--clm-NEON-FATES-NIWO RUN time=1151 (UNEXPECTED: expected FAIL)

[mvdebolskiy]

May be swap compilers between test-suites here? Since the test above already checks _D and Intel.

[mvdebolskiy]

Oh, now I see why you had baseline diffs and I did not. I was comparing ERI fix with the version where I've already removed (use_fates .and. .not. doalb) since you update coszen twice on coldstart and that update was not necessary. Not sure how big of a difference that makes actually. The only thing I notice now is that (get_nstep()==1) is on the both sides of the .or. so it can probably go into (use_fates .and. .not. doalb) => (use_fates .and. .not. doalb .and. get_nstep()==1)