-
Notifications
You must be signed in to change notification settings - Fork 321
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
Relax tolerance for truncating small snocan values in CanopyFluxes #2457
Conversation
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
There was a problem hiding this 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...
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. |
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. |
@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: |
Test results here |
@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. |
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
Testing in preparation for the merge izumi OK |
Relax tolerance for truncating small snocan values in CanopyFluxes See the PR ESCOMP#2457 for details.
Relax tolerance for truncating small snocan values in CanopyFluxes See the PR ESCOMP#2457 for details.
Relax tolerance for truncating small snocan values in CanopyFluxes See the PR ESCOMP#2457 for details.
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