Patch fusion crash in historic land only simulation fix #1579
+20
−4
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,18 @@ module EDCanopyStructureMod | |
|
|
||
| real(r8), parameter :: co_area_target_precision = 1.0E-9_r8 | ||
|
|
||
| ! Relative precision target used alongside the absolute targets above. | ||
| ! Area conservation checks compare differences of two large, similarly | ||
| ! sized areas (e.g. cohort crown areas, layer areas, patch areas). For | ||
| ! such differences a fixed absolute threshold becomes meaningless once | ||
| ! the operands are large, because a single rounding of an operand of | ||
| ! magnitude x already produces a residual of order ulp(x) ~ 2.2e-16*x. | ||
| ! To avoid spuriously tripping error checks on rounding noise, the | ||
| ! 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 | ||
|
|
||
| integer, parameter :: demotion_phase = 1 | ||
| integer, parameter :: promotion_phase = 2 | ||
|
|
||
|
|
@@ -564,7 +576,8 @@ subroutine PromoteOrDemote(site,patch,target_layer,phase,target_area) | |
|
|
||
| sumpd_area = 0._r8 | ||
| ic = 1 | ||
| do while( ic<=n_layer .and. (promdem_area-sumpd_area) > & | ||
| max(co_area_target_precision, rel_area_precision*promdem_area)) | ||
|
|
||
| cohort => layer_co(ic)%p | ||
|
|
||
|
|
@@ -611,7 +624,8 @@ subroutine PromoteOrDemote(site,patch,target_layer,phase,target_area) | |
| ! the cohort area within precision checks then fail | ||
|
|
||
|
|
||
| whole_or_part: if( ((layer_co(ic)%pd_area - cohort%c_area) > & | ||
| max(co_area_target_precision, rel_area_precision*cohort%c_area) ) .or. & | ||
| (layer_co(ic)%pd_area < 0._r8) ) then | ||
| write(fates_log(),*) 'negative,or more area than the cohort has is being promoted/demoted' | ||
| write(fates_log(),*) 'change: ',layer_co(ic)%pd_area | ||
|
|
@@ -620,7 +634,8 @@ subroutine PromoteOrDemote(site,patch,target_layer,phase,target_area) | |
| call endrun(msg=errMsg(sourcefile, __LINE__)) | ||
|
|
||
|
|
||
| elseif ( abs(layer_co(ic)%pd_area - cohort%c_area) < & | ||
|
Contributor
There was a problem hiding this comment. @glemieux noted this would be better as an "<=", I agree
Contributor
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 |
||
| max(co_area_target_precision, rel_area_precision*cohort%c_area) ) then | ||
|
|
||
| ! Whole cohort promotion/demotion | ||
| cohort%canopy_layer = cohort%canopy_layer + ilyr_change | ||
|
|
@@ -875,7 +890,8 @@ subroutine canopy_summarization( nsites, sites, bc_in ) | |
| call endrun(msg=errMsg(sourcefile, __LINE__)) | ||
| end if | ||
|
|
||
| if (currentPatch%total_canopy_area - (1._r8-imperfect_fraction)*currentPatch%area > & | ||
| max(area_error_1, rel_area_precision*currentPatch%area)) then | ||
| write(fates_log(),*) 'too much canopy in summary', s, & | ||
| currentPatch%nocomp_pft_label, currentPatch%total_canopy_area - (1._r8-imperfect_fraction)*currentPatch%area | ||
| call endrun(msg=errMsg(sourcefile, __LINE__)) | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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?
Uh oh!
There was an error while loading. Please reload this page.
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.
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...
cc @maritsandstad
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.
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.