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

Change all 'hite' to 'height' #1091

Merged
merged 3 commits into from
Oct 10, 2023
Merged

Change all 'hite' to 'height' #1091

merged 3 commits into from
Oct 10, 2023

Conversation

JessicaNeedham
Copy link
Contributor

@JessicaNeedham JessicaNeedham commented Sep 25, 2023

This PR changes all occurrences of ‘hite’ in FATES to ‘height’. There are no changes for users since it was already correct in the parameter file and history outputs.

At the software meeting today there seemed to be some fond feelings towards this historical quirk but no strong objections to fixing it.

Collaborators:

Expectation of Answer Changes:

No changes to users. Should be bfb.

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

All checklist items must be checked to enable merging this pull request:

Contributor

  • The in-code documentation has been updated with descriptive comments
  • The documentation has been assessed to determine if updates are necessary

Integrator

  • FATES PASS/FAIL regression tests were run
  • Evaluation of test results for answer changes was performed and results provided

Documentation

Test Results:

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

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

FATES baseline hash-tag:

Test Output:

Compiles and runs on Perlmutter CPU.

@rosiealice
Copy link
Contributor

I am cool with this, although I will miss it slightly.

@rgknox
Copy link
Contributor

rgknox commented Sep 28, 2023

I'm going to need some time sort through my feelings

@JessicaNeedham JessicaNeedham added the clean up Simple issues like typos, formatting, style guide alignment, etc. label Sep 28, 2023
@aswann
Copy link
Collaborator

aswann commented Sep 28, 2023

This gives me flashbacks to some very frustrated times greping to figure out how height was calculated in ED2. It only took 13 years to change it!

@rgknox
Copy link
Contributor

rgknox commented Oct 2, 2023

I think @aswann makes a compelling argument to change

Copy link
Contributor

@rgknox rgknox left a comment

Choose a reason for hiding this comment

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

Changes are straight forward, I did a double check through the code, things look good, thanks @JessicaNeedham

@ckoven
Copy link
Contributor

ckoven commented Oct 2, 2023

This seems great to me. One question is should we also change cohort%dbh to something like cohort%diameter or cohort%stem_diameter as well? DBH isn't always an accurate description of the variable (i.e. for grasses which use a basal diameter as the reference allometric variable).

In this tag, Jessica Needham adds a fix to nocomp initialization by adding in a call to height demography.
@glemieux
Copy link
Contributor

During regression testing a ran into a weird pio issue across all tests (including the one lone ctsm only testmod). I suspect this was a cheyenne glitch. I've double checked that the baseline runs fine. I'm running a single AllVars case again to confirm my suspicions.

@glemieux glemieux self-assigned this Oct 10, 2023
@glemieux
Copy link
Contributor

After fixing another hite variable that came in with deconflicting against the latest tags, I was able to successfully re-run the regression tests. All expected tests passed b4b.

cheyenne results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1091-hitefix

@glemieux glemieux merged commit a3048a6 into NGEET:main Oct 10, 2023
1 check passed
@JessicaNeedham JessicaNeedham deleted the hitefix branch January 11, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up Simple issues like typos, formatting, style guide alignment, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants