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

Satellite phenology mode fixes for the gnu compiler #838

Merged
merged 14 commits into from
Mar 2, 2022

Conversation

glemieux
Copy link
Contributor

@glemieux glemieux commented Feb 17, 2022

Description:

This fixes #832 by explicitly setting the cohort canopy layer to one when initializing cohorts during satellite phenology (SP) mode. In the course of correcting this issue it was also discovered that the SP mode was failing the exact restart test (ERS) due to FATES_FDI not being initialized (even though we force spitfire to be off during the SP runs). This history variable output is the only fire output that is a direct assignment, which is why I think it might not have been showing up for non-fire gnu compiler ERS runs. Setting the site-level fdi equal to zero in the zero_site subroutine corrects this issue.

I also realized that we don't specifically care about the hlm_is_restart in UpdateCohortBioPhysRates. We can just check if we are in SP mode.

Collaborators:

@peterdschwartz

Expectation of Answer Changes:

This should be b4b against the baseline except for the SatPhen testmod.

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:

The ERS gnu compiler test is not standard on Cheyenne or Cori, but I tested it for completeness sake:
/glade/u/home/glemieux/scratch/ctsm-tests/tests_spmode-gnufail-gnuzero

CTSM (or) E3SM (specify which) test hash-tag: ESCOMP/CTSM@bcb9735

CTSM (or) E3SM (specify which) baseline hash-tag: ESCOMP/CTSM@bcb9735

FATES baseline hash-tag: 477a8d5

Test Output: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr838-gnu_spmode_fix

@glemieux
Copy link
Contributor Author

All expected tests PASS b4b against the baseline. Test folder location:
/glade/u/home/glemieux/scratch/ctsm-tests/tests_pr838-gnu_spmode_fix

@glemieux
Copy link
Contributor Author

I'm running a last check on Cori using a version of this PR branch that is compatible with the current ELM-FATES API: https://github.com/glemieux/fates/commits/spmode-gnufix.

@glemieux
Copy link
Contributor Author

glemieux commented Feb 22, 2022

I'm running a last check on Cori using a version of this PR branch that is compatible with the current ELM-FATES API: https://github.com/glemieux/fates/commits/spmode-gnufix.

For some reason this failed on Cori. Final regression testing is on hold until I figure out what happened.

@glemieux
Copy link
Contributor Author

glemieux commented Feb 23, 2022

Ok, I think I see what the issue is here. There is another initialization issue: we're calling assign_cohort_SP_properties prior to setting vcmax25top with UpdateCohortBioPhysRates. The quick fix for this is to add a call to the latter during within the former; I've got a test for that queued on Cori. That said, I'm thinking about spending time to see if we can refactor this in some way. Doing the short term fix would work, but it seems inconsistent since there will be a downstream call to create_cohort that will NaN a whole bunch of cohort variables including vcmax25top.

I'm going to tag this as "Not Ready" and move it down the project board while I work on this.

@glemieux glemieux added the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label Feb 23, 2022
@glemieux
Copy link
Contributor Author

glemieux commented Mar 1, 2022

After reviewing options for refactoring the code and talking with @ckoven, I've moved ahead with implementing the simple immediate fix of assigning the vcmax25top values during sp mode initialization (via 5d40c3e). Implementing this update now resulting in a passing gnu compiler sp mode fates case. The output for that test case is here on Cori:
/global/homes/g/glemieux/scratch/e3sm-cases/satphenmatch-clfix_FDIfix-vcmaxfix3.fates-sci.1.50.0_api.17.0.0-v2.0.0-C3a1b5ed84b-Fb78153de.gnu

@glemieux glemieux removed the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label Mar 1, 2022
@rgknox
Copy link
Contributor

rgknox commented Mar 2, 2022

Tests OK:
/glade/scratch/rgknox/tests_0302-090114ch/

@rgknox rgknox merged commit 7edf030 into NGEET:master Mar 2, 2022
@glemieux
Copy link
Contributor Author

glemieux commented Mar 2, 2022

With the change from 5d40c3e the ReducedComplexSatPhen testmods will not be b4b against the baseline now. I confirmed that this is because prior to this change the vcmax25top value during initialization was zero everywhere (for the intel compiler). This can be seen by comparing the output from the following folders:

  • With 5d40c3e: /glade/u/home/glemieux/scratch/ctsm-tests/tests_spmode-pr838-revertdiag_wfix-more
  • With out 5d40c3e: /glade/u/home/glemieux/scratch/ctsm-tests/tests_spmode-pr838-revertdiag_wofix-more

@glemieux glemieux deleted the spmode-gnufix-master branch March 21, 2022 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Satellite phenology mode run fails with gnu compiler
2 participants