Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax tolerance for truncating small snocan values in CanopyFluxes #2457

Merged

Conversation

billsacks
Copy link
Member

Description of changes

The default of 1e-13 was letting through some state updates that look like they should have been truncated to 0: each time step, snocan was decreasing by about 12 orders of magnitude.

It looks like what's happening is that snocan is periodically getting set to a very small but non-zero value – maybe coming from a small value of liqcan (which probably needs its own truncate_small_values, which might solve this issue, but that's a matter for another time). Then the model seems to be "trying" to evaporate all of this small snocan in CanopyFluxes in the next time step: the fluxes have values that reduce snocan by something like 12 orders of magnitude. But that reduction isn't quite enough to trigger the truncate_small_values call that @slevis-lmwg introduced, since that uses the default relative epsilon of 1e-13. By loosening the tolerance on the epsilon value, we allow the state update to achieve what seems to be its "desired" reduction of the state to exactly 0 in a single time step.

I think the reason this was causing a failure in the tracer test is that eventually the snocan state got close to the smallest representable double precision value: the test failed when snocan reached 4e-296; the tracer in question is set up to be 10 orders of magnitude smaller, so it should have had a value of 4e-306 at this point. But that's getting very close to limit of representation of double precision, which is about 1e-308, so I think the tracer value lost precision and was rounded to 0, while the bulk value was still very slightly non-zero.

My hypothesis for why this just started appearing with the change that @ekluzek found in #2444 (comment) is that the new freezing soil temperatures probably cause veg temperatures to be below freezing, which causes snocan to come into play where it maybe didn't before.

Specific notes

Contributors other than yourself, if any:

CTSM Issues Fixed (include github issue #):

Are answers expected to change (and if so in what way)? YES - roundoff level changes expected for some (maybe many) tests (even those without water tracers)

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

Testing performed, if any:
Ran SMS_D_Ld10.f10_f10_mg37.I2000Clm50BgcCrop.izumi_intel.clm-tracer_consistency, verified that it PASSes now

The default of 1e-13 was letting through some state updates that look
like they should have been truncated to 0: each time step, snocan was
decreasing by about 12 orders of magnitude.

Addresses ESCOMP#2444
@ekluzek ekluzek added the enhancement new capability or improved behavior of existing capability label Apr 5, 2024
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks for figuring this out @billsacks.

I'm thinking for visibility this should go to master rather than ctsm5.2 since it's an answer change, and I'd like it to be more obvious than bound up in all the ctsm5.2 changes. But, I'll ask other opinions...

@billsacks
Copy link
Member Author

I'm thinking for visibility this should go to master rather than ctsm5.2 since it's an answer change, and I'd like it to be more obvious than bound up in all the ctsm5.2 changes. But, I'll ask other opinions...

That makes sense to me. I branched off of the 5.2 branch so that I could reproduce the issue and verify that this resolves it, but I agree with your approach here.

@ekluzek ekluzek changed the base branch from ctsm5.2.mksurfdata to master April 5, 2024 04:48
@ekluzek
Copy link
Collaborator

ekluzek commented Apr 5, 2024

I rebased to master, which means it now contains all of ctsm5.2, but that should be cleared up when ctsm5.2.0 is made on master.

@ekluzek ekluzek added this to the 2024 CESM June workshop milestone Apr 25, 2024
@ekluzek ekluzek added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Apr 25, 2024
@wwieder wwieder removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Apr 25, 2024
@wwieder wwieder added the testing additions or changes to tests label Jun 20, 2024
@slevis-lmwg
Copy link
Contributor

slevis-lmwg commented Jul 6, 2024

@billsacks, could you give me collaborator permissions, so that I may update this PR to the latest ctsm tag in preparation for merging to master?

Submitted on derecho:
./run_sys_tests -s aux_clm -c ctsm5.2.008 --skip-generate

@slevis-lmwg
Copy link
Contributor

Test results here
/glade/derecho/scratch/slevis/tests_0706-120617de
Diffs exceed roundoff, but likely because they grow over time.
@billsacks could you give your assessment (and blessing if appropriate)?

@ekluzek ekluzek removed their assignment Jul 9, 2024
@billsacks
Copy link
Member Author

@slevis-lmwg I have given you collaborator permissions on my fork. Thank you!

This PR now looks messed up. If just merging in the latest master doesn't clear things up, we could open a new PR with commit fef58b6 cherry-picked.

Given the minimal nature of these changes, I don't feel a need to look at the test results themselves: As you were seeing, I expect changes that grow from initially-roundoff-level to something bigger.

@slevis-lmwg slevis-lmwg added the PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete label Jul 11, 2024
Explicit A/C adoption

Code changes for adding an explicit air-conditioning (AC) adoption parameterization scheme in CLMU. This includes adding a new time-varying input variable (AC adoption rate, p_ac), changes to building energy calculations, and a toggle (new namelist variable urban_explicit_ac).

In this tag we keep the change off by default in order to show that answers do not change.

Fixes ESCOMP#2254 Explicitly representing air-conditioning adoption in CESM.

slevis resoloved conflicts:
doc/ChangeLog
doc/ChangeSum
…fix_tracer_test_v2

slevis resolved conflicts:
doc/ChangeLog
doc/ChangeSum
@slevis-lmwg
Copy link
Contributor

slevis-lmwg commented Jul 15, 2024

Testing in preparation for the merge
./run_sys_tests -s aux_clm -g ctsm5.2.012_slevis -c ctsm5.2.011

izumi OK
derecho OK

@slevis-lmwg slevis-lmwg merged commit 1d3b180 into ESCOMP:master Jul 16, 2024
2 checks passed
@slevis-lmwg slevis-lmwg deleted the ctsm5.2.mksurfdata_fix_tracer_test_v2 branch July 16, 2024 14:59
slevis-lmwg added a commit to chrislxj/ctsm that referenced this pull request Jul 16, 2024
Relax tolerance for truncating small snocan values in CanopyFluxes

See the PR ESCOMP#2457 for details.
slevis-lmwg added a commit to HuiWangWanderInGitHub/CTSM that referenced this pull request Jul 16, 2024
Relax tolerance for truncating small snocan values in CanopyFluxes

See the PR ESCOMP#2457 for details.
glemieux added a commit to samsrabin/CTSM that referenced this pull request Jul 17, 2024
Relax tolerance for truncating small snocan values in CanopyFluxes

See the PR ESCOMP#2457 for details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete testing additions or changes to tests
Projects
Status: Done (non release/external)
Status: Done
Development

Successfully merging this pull request may close these issues.

Failing water isotope test on the ctsm5.2 branch
4 participants