Patch fusion crash in historic land only simulation fix#1579
Conversation
…elative error indicating differences below noise
github-project-automation
Bot
moved this to Finding Reviewers
in FATES Pull Request Planning and Status
github-project-automation
Bot
moved this to To triage
in FATES Historical transient bug tracker
| ! comparisons below use max(absolute_target, rel_area_precision*|operand|). | ||
| ! rel_area_precision is set to relative machine precision for r8 so that | ||
| ! a difference below machine precision is never treated as a real error. | ||
| real(r8), parameter :: rel_area_precision = 1.0E-15_r8 |
There was a problem hiding this comment.
Since this is supposed to match the general precision of math on most compilers, maybe we should move this to FatesConstantsMod , give it a more general name, and use it in more places?
There was a problem hiding this comment.
Also, we should not be so strict, and expect machine precision exactly. I think we should drop this down at least an order of magnitude, if not 2...
There was a problem hiding this comment.
Actually I did do it by putting it in FatesConstantsMod for later PRs, I agree it should move here also for this (see here: NorESMhub#74).
I'm not sure what you mean, do you want it to be lower as in 1.e-17 or 1.e-13. I would argue quite strongly against the former, asl that would allow you to let noise through, the point of that is to not do that. I would be fine with going to a larger number like 1e-13, but it is easiest to argue the machine precision numbers.
|
|
||
|
|
||
| elseif ( abs(layer_co(ic)%pd_area - cohort%c_area) < co_area_target_precision ) then | ||
| elseif ( abs(layer_co(ic)%pd_area - cohort%c_area) < & |
There was a problem hiding this comment.
@glemieux noted this would be better as an "<=", I agree
There was a problem hiding this comment.
I mean, for all practical purposes there is no difference between a < or <= comparison here. Chances that you are bang on to 15 digits are virtually nill
|
Closing in favor of merging via #1583 or similar direct pull request. |
github-project-automation
Bot
moved this from To triage
to Done
in FATES Historical transient bug tracker
3 participants
Description:
Changing EDCanopyStructureMod.F90 to have relative rather than absolute error comparisons so that errors are not triggered by differences which are in the noise of r8 precisions numbers
Addresses NorESMhub/CTSM#223. Cherry-picked from NorESMhub/CTSM#66.
Collaborators:
@maritsandstad
Expectation of Answer Changes:
Description of generative AI usage (as necessary)
Checklist
If this is your first time contributing, please read the CONTRIBUTING document.
All checklist items must be checked to enable merging this pull request:
Contributor
Integrator
If satellite phenology regressions are not b4b, please hold merge and notify the FATES development team.
Documentation
Test Results:
CTSM (or) E3SM (specify which) test hash-tag:
CTSM (or) E3SM (specify which) baseline hash-tag:
FATES baseline hash-tag:
Test Output: