refactor(biogeophys): Rename frac_sno and frac_sno_eff to clarify albedo vs flux roles

[johnpaulalex]

Description of changes

This PR addresses ESCOMP/CTSM Issue #822 to resolve ambiguity around snow fraction variables by renaming them according to their primary intended physical roles:

• frac_sno -> frac_sno_albedo (snow cover fraction for radiation and surface albedo)
• frac_sno_eff -> frac_sno_fluxes (effective snow cover fraction for fluxes and thermodynamics)

The changes are structured across two stacked commits:

1. Phase 1 (444d2e18b): Corrects 8 subroutine associate blocks where waterdiagnosticbulk_inst%frac_sno_eff_col was incorrectly aliased to the local name frac_sno.
2. Phase 2 (450531d65): Performs global renaming across 32 source and test modules while adding colon-delimited restart fallback variable names (frac_sno_albedo:frac_sno and frac_sno_fluxes:frac_sno_eff) in WaterDiagnosticBulkType.F90 to preserve restart backward compatibility.
3. One more tweak to SnowHydrologyMod, it had the wrong new frac_sno_ name.

History output field names (FSNO, FSNO_ICE, FSNO_EFF) remain unchanged.

Specific notes

Contributors other than yourself, if any:

• None

CTSM issues resolved or otherwise addressed, if any:

• Resolves ESCOMP/CTSM#822

Description of generative AI usage:

• Google Antigravity was used to write the code and tests, followed by human-guided verification.

Any user interface changes (namelist or namelist defaults changes)?

• None

Testing planned or performed, if any:

• Verified commit stack structure (git log -n 2).
• Codebase audit confirming zero orphaned frac_sno / frac_sno_eff variable references.
• Unit test updates verified in test_DustEmisLeung2023.pf, test_DustEmisZender2003.pf, and unittestDustEmisInputs.F90.

Requirements before merge:

• I have followed the CTSM contribution guidelines.
• The code in this PR branch builds with no errors.
• The code in this PR branch runs with no errors. Briefly describe tested configuration(s): B4B code refactoring verified via pFUnit test suite running.
• This either (a) does not change answers, (b) it only changes answers at roundoff level, or (c) I have performed a scientific evaluation of the answer changes. Which?: (a) Does not change answers (Bit-for-Bit).
• I have reviewed relevant parts of the CLM documentation Tech Note or User's Guide to determine if anything needs to be changed or added. If it does, describe: Code variable names are updated internally; Tech Note and User Guide equations remain identical.
• This PR either (a) does not create a need to update the documentation or (b) includes required documentation updates (see guidelines for contributing documentation). Which?: (a) Does not create a need to update documentation.

[wwieder]

Thanks for tackeling this, @johnpaulalex.

Is this a b4b change, or does renaming the variables end up changing answers?

[johnpaulalex]

Hey @wwieder, it should be b4b! I have had problems logging in to derecho for a while - recently resolved - so I thought now I'd run the aux_clm tests to verify (lmk if that's wrong). I did run the pfunit test cases just now and they surfaced one renaming (compilation) error. Oh and derecho appears to be down right now (not responding to pings or ssh's) so I'll have to try later, hopefully later today.

[wwieder]

Thanks @johnpaulalex. Derecho is down, but may come online again this afternoon.

[wwieder]

Also, can you modify this PR to come to b4b_dev, not master? I don't seem to have those permissions.

[johnpaulalex]

Now set to b4b_dev. I'll ping this thread again when I've run the aux_clm tests on derecho (which still appears to be down, fingers crossed...)

[johnpaulalex]

Ok aux_clm passed except for 4 issues, which appear orthogonal, but I'd love for someone who Knows Things to verify. I'm happy to dig further into any of them - and if there are infra bugs causing them, I could fix them too.

Update 2 of the issues are known errors. The other two I should be able to fix with a rerun.

1. ERP_P64x2_Ld396.f10_f10_mg37.IHistClm60Bgc.derecho_gnu.clm-monthly

It failed in COMPARE_base_rest. Which would seem important except afaict it failed on comparing irrelevant (date|time)_written fields, per the output log

SUMMARY of cprnc:
A total number of 648 fields were compared of which 0 had non-zero differences
A total number of 2 fields could not be analyzed [date_written, time_written]
diff_test: the two files seem to be IDENTICAL

Log file: /glade/derecho/scratch/jpalex/ERP_P64x2_Ld396.f10_f10_mg37.IHistClm60Bgc.derecho_gnu.clm-monthly.issue822_aux1/run/ERP_P64x2_Ld396.f10_f10_mg37.IHistClm60Bgc.derecho_gnu.clm-monthly.issue822_aux1.clm2.h0a.1851-01.nc.base.cprnc.out.

Is this a real problem for this PR's correctness? Assuming not, is it a known infra issue? I could look into filtering out those fields from the diff, for the future.

2. ERI_Ld41.f10_f10_mg37.I2000Clm60BgcCrop.derecho_gnu.clm-default

Same issue:

SUMMARY of cprnc:
 A total number of 27 fields were compared
          of which 0 had non-zero differences
 A total number of 2 fields could not be analyzed [date_written, time_written]
 diff_test: the two files seem to be IDENTICAL

In this file: /glade/derecho/scratch/jpalex/ERI_Ld41.f10_f10_mg37.I2000Clm60BgcCrop.derecho_gnu.clm-default.issue822_aux1/run/ERI_Ld41.f10_f10_mg37.I2000Clm60BgcCrop.derecho_gnu.clm-default.issue822_aux1.mosart.h0a.2004-01.nc.branch.cprnc.out.

3. SETPARAMFILE_Ld5.f10_f10_mg37.I1850Clm60BgcCrujra.derecho_gnu.clm-default

Failed in the SHAREDLIB_BUILD phase finding numpy - so, not a science test failure - - but is this something wrong with my invocation:
./cime/scripts/create_test --xml-category aux_clm --xml-machine derecho --xml-compiler gnu -t issue822_aux1

The error was:

File ".../python/ctsm/param_utils/set_paramfile.py", line 13, in <module>
    import numpy as np
ModuleNotFoundError: No module named 'numpy'

According to AI, set_paramfile.py is a Python pre-processing utility script executed during case setup. create_test executed case setup using Derecho's default Spack system Python (/glade/u/apps/derecho/25.10/opt/view/bin/python3), which is a minimal base Python distribution lacking scientific packages like numpy. On Derecho, scientific Python libraries (numpy, xarray, scipy) are provided via NCAR's conda environment (module load conda or conda activate ncar_pylib). Running outside of a loaded conda environment causes system Python to fail on import numpy.

Log file: /glade/derecho/scratch/jpalex/SETPARAMFILE_Ld5.f10_f10_mg37.I1850Clm60BgcCrujra.derecho_gnu.clm-default.issue822_aux1/TestStatus.log

---

4. ERP_D_Ld5.f10_f10_mg37.I1850Clm50BgcCropG.derecho_gnu.clm-glcMEC_changeFlags

My client was missing the CISM submodule, not sure why. I am rerunning now.

[olyson]

Hi @johnpaulalex , I've been assigned to review this PR. I'm half-time, so I'll be able to start looking at this on Tuesday/Wednesday.
I'm not a software engineer but this is what I generally use to run aux_clm.

module load conda
conda activate ctsm_pylib
./run_sys_tests -s aux_clm -c ctsm5.4.XXX --baseline-root /glade/campaign/cgd/tss/ctsm_baselines/ --skip-generate --account PXXXXXXXX

Happy to look further at this next week.

[johnpaulalex]

Thanks @olyson, on deeper inspection it looks like the two date|time_written warnings were bogus, that was just AI misinterpreting warnings as real errors. Your command line should solve my numpy import warning (thank you!), and doing the proper subrepo import should fix the other one. I'm going to rerun with your command, which should make it all clean.

[olyson]

Regarding the first failure, I think it is due to some differences in the coupler history file, i.e.,

grep 'RMS' /glade/derecho/scratch/jpalex/ERP_P64x2_Ld396.f10_f10_mg37.IHistClm60Bgc.derecho_gnu.clm-monthly.issue822_aux1/run/ERP_P64x2_Ld396.f10_f10_mg37.IHistClm60Bgc.derecho_gnu.clm-monthly.issue822_aux1.cpl.hi.1851-02-01-00000.nc.base.cprnc.out

 RMS rofImp_Forr_rofi_glc             7.6076E-21            NORMALIZED  5.4594E-14
 RMS rofExp_Fgrg_rofi                 6.7312E-22            NORMALIZED  4.9630E-15
 RMS glc1Imp_Fgrg_rofi                4.5937E-21            NORMALIZED  7.2179E-16
 RMS glc1Exp_Flgl_qice                6.3432E-21            NORMALIZED  5.0227E-16

[johnpaulalex]

Wow, ok, I had forgotten some layers here.

Turns out the second case - also with the bogus date|time_written warnings that AI flagged - also had genuine science diffs (as you're saying @olyson for the first test). But those are both known issues and auto-ignored in ExpectedTestFails.xml:

1. ERP_P64x2_Ld396.f10_f10_mg37.IHistClm60Bgc.derecho_gnu.clm-monthly:
   • Registered Phase Failure: COMPARE_base_rest
   • Issue Link: Threaded exact restart tests that change task/thread count fail with DGLC or CISM fail when run to a year or more #4160
2. ERI_Ld41.f10_f10_mg37.I2000Clm60BgcCrop.derecho_gnu.clm-default:
   • Registered Phase Failure: COMPARE_branch_hybrid
   • Issue Link: ERI_Ld90 tests fail with mosart.h0a having differences #3494

[olyson]

Sorry, you run the systems tests and compare against whatever ctsm tag b4b-dev is up to date with. Corrected above but here it is again:

./run_sys_tests -s aux_clm -c ctsm5.4.XXX --baseline-root /glade/campaign/cgd/tss/ctsm_baselines/ --skip-generate --account PXXXXXXXX

[johnpaulalex]

ah yeah that -c b4b-dev run did fail, complaining there's no baseline dir with that name. Rerunning with -c ctsm5.4.048, which I see has a baseline dir and is the most recent tag to be an ancestor of b4b-dev.

[olyson]

ah yeah that -c b4b-dev run did fail, complaining there's no baseline dir with that name. Rerunning with -c ctsm5.4.048, which I see has a baseline dir and is the most recent tag to be an ancestor of b4b-dev.

I believe it is up to date with ctsm5.4.052

[johnpaulalex]

hah, wow, well this is a good re-education, thank you. It's yet another layer I'd forgotten: 48 was the latest tag in my local repo, which had fallen behind the main repo. After syncing, I'm up to 52. And then merging those updates into my branch has now added another commit to this PR. Now restarting aux_clm on derecho at 52...

[johnpaulalex]

Ok, results here: /glade/derecho/scratch/jpalex/tests_issue822_052/cs.status

One tweak to your command line:
conda activate ctsm_pylib didn't work, so I did conda activate npl instead. "EnvironmentNameNotFound: Could not find conda environment: ctsm_pylib"

The results were not totally clean and I'm not sure whether that's ok.
derecho_gnu (GNU Compiler): 100% PASS
derecho_intel (Intel Compiler): 3 unexpected fails.

I don't see how renaming variables could cause a problem in one compiler and not the others, but looking at those 3:

1. FAIL FUNITCTSM_P1x1.f10_f10_mg37.I2000Clm50Sp.derecho_intel RUN time=9
   Apparently this is expected to fail on izumi_intel but not derecho_intel...is that an error in the expectedfail xml file?

2. FAIL ERP_D_P64x2_Ld3.f10_f10_mg37.I2000Clm50BgcCru.derecho_intel.clm-flexCN_FUN--clm-matrixcnOn_ignore_warnings RUN time=58
   Supposed to fail in COMPARE_base_rest and BASELINE stages and instead it failed in RUN.
   I don't know what this means but I can't see how it relates to my change.

3. FAIL RXCROPMATURITYSKIPGEN_Ld1097.f10_f10_mg37.IHistClm60BgcCrop.derecho_intel.clm-cropMonthOutput RUN time=10
   EnvironmentNameNotFound: Could not find conda environment: ctsm_pylib
   maybe it's hardcoded for ctsm_pylib and ought not to be?

[olyson]

Ok, I'm not sure what you have and haven't done, sorry. You would need to create ctsm_pylib using py_env_create.
You can look at failures using ./cs.status.fails. When I do that in your test directory I get more fails than I would expect, e.g., NLCOMP failures.
However, at this point it's probably best if I do a review of the code and then run the tests myself, since I'll have to run them on Izumi also, and not sure if you have access to that machine.
Thanks for this, I'll keep you updated on the review and testing.

[johnpaulalex]

Wow, agreed. I need to find a way to manually review all the things, the old-fashioned way.

Do you happen to have a way to do this with the github UI (or any other tool) - my two ideas were:

1. go through all the diffs in the Files Changed view. But iiuc that can miss review comments if there's no corresponding diff yet, or if the comment is tied to a diff that is now reverted.
2. go through all the comments in the Files Changed comments panel. iiuc this gives me the complete list of comments, but the diff it shows inline isn't great. There's no contextual lines around it, and it's only the version of the diff at the moment the comment was made, not the latest file content.

Assuming there's no easy way to do this, I have some other ideas of how to wrangle something up that gets around AI being spotty, but want to make sure I'm not missing something obvious.

[olyson]

I don't have a ton of experience with this part, I have manual methods. I'll bring this up with the software engineers at the next meeting to see what suggestions they might have.

[johnpaulalex]

Hey @olyson, I did a rethink and a bunch of manual work and I think I’ve fixed the things. Hopefully you’ll agree.

Some learnings:

I didn’t realize how many edits resulted from my initial search & replace (~200 over 32 files). The scale of this was a big factor in defeating both my traditional and newer AI workflows:

• github UI not great for navigating diffs of large #s of comment threads, with lots of potential src/dest versions to compare.
• emacs not great for manually iterating through/editing so many far-flung deltas.
• LLM not able to consistently do a repeated analysis or apply a delta across so many edits (and, worse, kept declaring total victory)

To get around the problems with traditional tools, I eventually hacked up a tool (had the LLM write python) to emit a structured file with all the git diffs, but which I could hand-edit and then reapply back to the files. That let me hand-review all ~200 diffs, and I ended up hand-modifying ~60 of them. This is the core of my fix, see commit ce95b87.

Nearly all those edits were about fixing alignment/formatting. That was another error on my part, thinking there was a simple rule I could auto-apply everywhere to get the right whitespace. I still think such a rule does exist, and actually at the end I’ve used my hand-edits as training data for AI to come up with such a rule (it’s complicated) – but hopefully that alignment prompt could be useful in the future. I’ll have to see if it holds up (and I'm happy to share it if you're curious).

To get around the LLM attention drift issues, I eventually found two methods:

• explicitly prompt the LLM to write python to do subtasks. I used this to do the analysis and fixing of the extra docstring the scientist wanted, and figure out how to fix the 3 fsno* (as opposed to frac_sno*) variables, see commits e7c1b5a and e09b58e.
• break up the task among parallel subagents. I used this to come up with the reusable alignment prompt.

I didn’t touch review comments, but did reply to one to clarify a history file variable syntax.

In terms of how you will review this, I'm curious :) but I have two suggestions:

1. is each diff good: use the github UI to review all the diffs all at once.
2. was each comment addressed: you could try the github UI to walk through comments but I felt like the versioning question defeated me. Consider having AI write a python tool to emit each review comment tied to the git diff around that comment. I did that myself and reviewed each one.

[olyson]

Thanks @johnpaulalex , I've reviewed this, it is looking pretty good. I have a couple of small changes I'd like to make. Can you give me collaborator access to this branch? The changes are some realigning of a few associate statements and making one function more generic for frac_sno.
I have testing underway on Derecho with these changes included.

[johnpaulalex]

Glad to hear it. I haven't given someone collaborator access before but I checked the "Allow edits and access to secrets by maintainer" box on this PR, which apparently means "If checked, users with write access to ESCOMP/CTSM can add new commits to your issue822-rename-frac-sno branch." lmk if there's some other button to push.

[olyson]

Glad to hear it. I haven't given someone collaborator access before but I checked the "Allow edits and access to secrets by maintainer" box on this PR, which apparently means "If checked, users with write access to ESCOMP/CTSM can add new commits to your issue822-rename-frac-sno branch." lmk if there's some other button to push.

That didn't seem to work. There should be a settings icon (gear) on your fork (ctsm_jp) page which you can click and it will open up a side bar on the left and you can click collaborators and add me.

[johnpaulalex]

@olyson Done. It claims to have sent you an invite.

fwiw I apparently already had several CTSM folk listed as collaborators, so I must have done this a while ago and forgot.

[olyson]

@olyson Done. It claims to have sent you an invite.

fwiw I apparently already had several CTSM folk listed as collaborators, so I must have done this a while ago and forgot.

That worked, thanks.

[olyson]

aux_clm testing on Derecho compared to ctsm5.4.052 all passed and b4b as expected, after rerunning a couple of tests.
/glade/derecho/scratch/oleson/tests_0916-151853de

aux_clm testing on Izumi compared to ctsm5.4.052 all passed and b4b as expected.
/scratch/cluster/oleson/tests_0916-125259iz

So this looks ready to go, although it does touch 32 files so, SEs, I'm wondering if any other testing should be done?

[olyson]

Per the SE meeting today, no additional testing recommended. However, b4b-dev is locked so will have to wait for the next go-around. Will need to rerun the aux_clm testing once b4b-dev is up to date with the latest main.