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

refactor of LAI-by-patch-age history (FATES_LAI_AP) #1174

Merged
merged 5 commits into from
May 1, 2024

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Mar 19, 2024

Description:

This is a bug fix. The FATES_LAI_AP diagnostic did not seem to be written correctly. Also, the variables should match FATES_LAI, when summed over patch area weights.

FATES_LAI = sum( FATES_LAI_AP * FATES_PATCHAREA_AP)

Fixes: #1170
Fixes: #1183

Collaborators:

@samsrabin

Expectation of Answer Changes:

This should change both FATES_LAI and FATES_LAI_AP, and a new variable FATES_ELAI is now available.

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:

@samsrabin
Copy link
Contributor

Looks like this works for me!
screenshot_0211

samsrabin added a commit to samsrabin/fates that referenced this pull request Mar 22, 2024
@rgknox
Copy link
Contributor Author

rgknox commented Mar 25, 2024

I might also add the fix to #1170 into this, since it's also trivial

@glemieux glemieux requested review from samsrabin and glemieux and removed request for samsrabin April 1, 2024 16:09
Copy link
Contributor

@JessicaNeedham JessicaNeedham left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @rgknox and @samsrabin

! Normalize crown-area weighted height
if(site_ca>nearzero)then
hio_ca_weighted_height_si(io_si) = hio_ca_weighted_height_si(io_si)/site_ca
end if
Copy link
Contributor

Choose a reason for hiding this comment

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

BA weighted height is set to zero if site_ba is not > nearzero. Seems like it would be a very small number anyway but do we want the same thing for CA weighted height?

Also the comment on 2867 could be updated to say that ca_weighted_height and ba_weighted_height are normalized outside of the patch loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it out because we zero everything in an automated function anyway, we don't actually need to zero the ba weighted output. But now that you bring the point up, its probably confusing to anyone reading the code why we do things differently. I think its better to have these outputs processed consistently to avoid confusion. I'll do a look through and see if I can make things more consistent in other places as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that makes sense.

@rgknox
Copy link
Contributor Author

rgknox commented Apr 29, 2024

tests ok on derecho, some diffs, and the diffs are related only to the changed output variables

@glemieux glemieux closed this Apr 29, 2024
@glemieux glemieux reopened this Apr 29, 2024
@rgknox rgknox merged commit bd75295 into NGEET:main May 1, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants