Skip to content
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

Fix mean vegetation temperature output for exact restarts #914

Merged
merged 9 commits into from
Oct 12, 2022

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

do i_pft = 1,numpft
sites(s)%use_this_pft(i_pft) = rio_use_this_pft_sift(io_idx_co_1st+i_pft-1)
sites(s)%area_pft(i_pft) = rio_area_pft_sift(io_idx_co_1st+i_pft-1)
enddo

! 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
Projects
None yet
2 participants