Skip to content

Add Tolerance Error Check in EDPatchDynamicsMod.F90: spawn_patches()#710

Merged
rgknox merged 6 commits into
NGEET:masterfrom
JoshuaRady:Disturbance_Check_Fix
Apr 13, 2021
Merged

Add Tolerance Error Check in EDPatchDynamicsMod.F90: spawn_patches()#710
rgknox merged 6 commits into
NGEET:masterfrom
JoshuaRady:Disturbance_Check_Fix

Conversation

@JoshuaRady

Copy link
Copy Markdown

Description:

This change addresses the problem described in issue #709 by adding a tolerance to an error check in EDPatchDynamicsMod.F90: spawn_patches() that allows the disturbance area to exceed 1.0 by minuscule amounts due to floating point math.

I added a local constant in EDPatchDynamicsMod.F90: spawn_patches() for the tolerance amount but it may be preferable to use a value defined in another module. There are several candidates but I was not clear on the intended used of these. Also given the minor nature of this change it could perhaps be combined with another issue. In any case this solution does work and serves to confirm the nature of the problem.

Collaborators:

I consulted with Gregory Lemieux (@glemieux) on this issue on Slack and he suggested the appropriate test to confirm that this was in fact a precision issue.

Expectation of Answer Changes:

This change should not be answer changing for simulations that would have passed the error check anyway. It should prevent runs from being terminated that would otherwise fail.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
  • I have read the CONTRIBUTING document.
  • FATES PASS/FAIL regression tests were run
  • If answers were expected to change, evaluation was performed and provided

Note: I created this branch prior to reading the contribution guidelines and the branch name does not conform to the naming conventions. My apologies for that.

Test Results:

I have not performed regression tests but I have performed basic tests to confirm the changes work as intended and can provide details if needed. I subsequently incorporated the change into my working branches and have not had any failures since. The change is only two lines. I'm glad to help with further testing if deemed necissary.

Testing was performed with:
CTSM baseline hash-tag: fates_next_api release-clm5.0.30-143-gabcd5937 / abcd5937
FATES baseline hash-tag: sci.1.41.0_api.13.0.1 / 61a751c

The branch was subsequently merged to the head of master.

@rgknox

rgknox commented Nov 17, 2020

Copy link
Copy Markdown
Contributor

@JoshuaRady would the constant define here:
https://github.com/NGEET/fates/blob/master/main/FatesConstantsMod.F90#L79-L88
be useful in this context?

@JoshuaRady

Copy link
Copy Markdown
Author

Yes, this seems like the appropriate constant. I have never seen a value with less than 14 zeros is my testing so this will work here. I can update the branch. Will the pull request advance to the head of the branch?

@glemieux

Copy link
Copy Markdown
Contributor

Yes, this seems like the appropriate constant. I have never seen a value with less than 14 zeros is my testing so this will work here. I can update the branch. Will the pull request advance to the head of the branch?

Yup, you just need to push to your branch and it will update the PR. You can also go ahead turn this into a regular PR from a draft. I'm not sure we really use draft PRs; we kind of expect there to be updates to the PR as we get reviews like Ryan's comments.

@JoshuaRady JoshuaRady force-pushed the Disturbance_Check_Fix branch from 384d006 to 228039b Compare March 7, 2021 21:29
@JoshuaRady JoshuaRady marked this pull request as ready for review March 27, 2021 20:55
@JoshuaRady

Copy link
Copy Markdown
Author

I have implemented the suggested changes from @rgknox and have brought this branch up to date with the head of FATES master. I have confirmed that the fix performs as it did prior to these updates. I believe this is ready for review.

@rgknox

rgknox commented Apr 12, 2021

Copy link
Copy Markdown
Contributor

All expected tests PASS:
/glade/scratch/rgknox/clmed-tests/jrady-tolerance-ctsm5.1.dev025-C2094348b-F5321daf2.fates.cheyenne

@rgknox

rgknox commented Apr 13, 2021

Copy link
Copy Markdown
Contributor

Ran a 70 year smoke test, cold-start, f10_f10, using a merge of this branch and #733. I mostly did this because its been a while since we've tested a long global run, but I also wanted to make sure that having 0 LAI values didn't break anything. I merged this branch into the test because why not. Everything passed, run completed. Merging.

@rgknox rgknox merged commit 832dc2f into NGEET:master Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error Check issue in EDPatchDynamicsMod.F90: spawn_patches() causes problem with logging module.

3 participants