Skip to content

Conversation

@glemieux
Copy link
Contributor

@glemieux glemieux commented Oct 5, 2022

Addresses #908 and addresses #911.

Description:

The FATES_TVEG and FATES_TVEG24 history output variables were found to have non-b4b restarts when testing the aux_clm suite for ESCOMP/CTSM#1849. It was found that both had issues with satellite phenology restarts due to mishandling of the bareground patch area_pft and patch iteration during running means update, respectively. This pull request adds in the necessary checks for the bareground patch and conduct site-level calculations, as needed.

Collaborators:

@rgknox

Expectation of Answer Changes:

Potentially, for satellite phenology ERS tests only

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

Test Results:

B4B aside from expected failures

CTSM (or) E3SM (specify which) test hash-tag: c6d8032c9

CTSM (or) E3SM (specify which) baseline hash-tag: c6d8032c9

FATES baseline hash-tag: fates-sci.1.59.6_api.24.1.0-ctsm5.1.dev111

Test Output:

/glade/u/home/glemieux/scratch/ctsm-tests/tests_pr914-2


! calculate the bareground area for the pft in no competition modes
if (hlm_use_nocomp .eq. itrue) then
if (sum(sites(s)%area_pft(1:numpft)) .lt. area) then
Copy link
Contributor

@rgknox rgknox Oct 6, 2022

Choose a reason for hiding this comment

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

should we compare this to a tolerance instead of having an exact match? like

abs(sum(x) - area) > nearzero

or if we are sure it will be less:

area-sum(x) > nearzero

Copy link
Contributor Author

@glemieux glemieux Oct 6, 2022

Choose a reason for hiding this comment

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

That's a good idea. I was lazy and basically copy/pasted this from EDInitMod. Looking back at the set_site_properties procedure, I think we probably should add the check there to make sure the sum is not larger than area.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I add a check to make sure the sum is never greater that area during prior to the get_restart_vectors, do you think that's sufficient since we don't update area_pft after the initialization of the run?

Copy link
Contributor Author

@glemieux glemieux Oct 6, 2022

Choose a reason for hiding this comment

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

After reviewing the checks for the variables that set area_pft . We check that hlm_pft_map sums to unity in EDPftvarcon and that pft_areafrac does the same in clmfates_interfacemod. So we shouldn't be able to get a value for sum of area_pft greater than area during set_site_properties (and presumably during restart).

@glemieux
Copy link
Contributor Author

glemieux commented Oct 6, 2022

I realized that the restart calculation check doesn't need to happen for all no comp modes; its restricted only to fixed biogeo + nocomp modes. Updated per 7ba71f5

@glemieux
Copy link
Contributor Author

Fates regression tests run against baseline fates-sci.1.59.6_api.24.1.0-ctsm5.1.dev111are showing b4b aside from expected failures. Folder location on cheyenne is here: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr914-2

Note that this ctsm5.1dev111 baseline was generated showing diffs against fates-sci.1.59.6_api.24.1.0-ctsm5.1.dev106. I still need to double check that these are expected DIFFs.

@glemieux
Copy link
Contributor Author

glemieux commented Oct 10, 2022

Note that this ctsm5.1dev111 baseline was generated showing diffs against fates-sci.1.59.6_api.24.1.0-ctsm5.1.dev106. I still need to double check that these are expected DIFFs.

Comparing the fates-sci.1.59.6_api.24.1.0-ctsm5.1.dev106 against fates-sci.1.59.6_api.24.1.0-ctsm5.1.dev111 is b4b (aside from fieldlist diffs due to dev108 update). This confirms that the DIFFs are due to the externals updates from ctsm5.1.dev107. This comparison can be found:
/glade/u/home/glemieux/scratch/ctsm-tests/tests_pr914-check_baseline-dev107v111

This PR should be good to go.

@glemieux glemieux merged commit d63b8d2 into NGEET:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

software: bug Bug that is specific to software

Projects

None yet

2 participants