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

Remove memory and lai variables #887

Merged
merged 12 commits into from
Sep 14, 2022
Merged

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Jul 29, 2022

Description:

The memory (laimemory, sapwmemory and structmemory) and lai terms are unecessary. We don't need the memory terms because we can use the allometry functions to calculate targets. We don't need the cohort level lai and sai variables because we can derive them from treelai, treesai, crown area and canopy area.

Fixes #541

I also removed the smooth leaf distribution code from the canopy vertical profile routine because that is old and unmaintained and is triggering lots of compiler warnings.

Collaborators:

Expectation of Answer Changes:

I expect small but subtle differences in results due to the laimemory stuff. The flushing target was previously laimemory, which was the target when the leaves dropped. Now the target is just re-caclulated when the leaves are ready to flush. Its possible that a trimming event would had happened in between, or perhaps due to canopy promotion/demotion/fusion there was a small change in dbh to maintain mass conservation, which would also trigger a difference in the target.

Checklist:

  • 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:

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

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

FATES baseline hash-tag:

Test Output:

@rosiealice
Copy link
Contributor

This is great @rgknox. Such a simple solution! I'm struggling to remember why it didn't work like this in the first place. How does it work with crown damage? I guess we just change the target?

@rgknox
Copy link
Contributor Author

rgknox commented Jul 29, 2022

right, crown damage level is remembered, and that instructs the allometry target

@rgknox rgknox requested review from mpaiao and rosiealice July 31, 2022 16:10

enddo !currentCohort
if( (currentCohort%treelai+currentCohort%treesai) > 0._r8)then
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent with other places in the code, should we use variable "nearzero" instead of 0._r8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has been changed to use nearzero

call PRTDeciduousTurnover(currentCohort%prt,ipft, &
leaf_organ, leaf_drop_fraction)

if(prt_params%woody(ipft).ne.itrue)then

currentCohort%sapwmemory = sapw_c * stem_drop_fraction
currentCohort%structmemory = struct_c * stem_drop_fraction

Copy link
Contributor

Choose a reason for hiding this comment

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

Another good reason to drop the memory variables: leafmemory was the prior leaf biomass, whereas sapwmemory/structmemory were the amount of woody biomass lost. I actually had changed the definition of sapwmemory/structmemory in #801 to the same as leafmemory because I found the original definitions very confusing...

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 saw that too, not sure if this will come up on tests, but glad its correct now

write(fates_log(),*) ' phen_stem_drop_fraction: ', EDPFtvarcon_inst%phen_stem_drop_fraction(ipft)
write(fates_log(),*) ' Aborting'
call endrun(msg=errMsg(sourcefile, __LINE__))
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.

Good idea to prevent the model to run if this combination is not acceptable.

Copy link
Contributor

@mpaiao mpaiao left a comment

Choose a reason for hiding this comment

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

@rgknox I checked the changes and they look all good to me, I just made one really minor suggestion in one "if" block to make the code a bit less likely to run into floating point exceptions.

This tag includes some simple improvements to math operations in high-frequency history diagnostics
@glemieux
Copy link
Contributor

glemieux commented Aug 16, 2022

Testing on Cheyenne is resulting in DIFFs across all tests except the clm50 non-fates test (expected) and the 1x1_brazil satellite phenology test. Looking at the other satellite phenology test mods with DIFFs, I'm seeing RMS values starting at the second time step for respiratory variables, FATES_LBLAYER_COND, the GPP, NPP, and NEP variables, and FATES_STOMATAL_COND. I'm not sure if this is expected for these changes. @rgknox thoughts?

Test folder location: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr887

@rgknox
Copy link
Contributor Author

rgknox commented Aug 16, 2022

yes, there should be diffs, some of it is related to what @mpaiao noted, in how we previously defined memory for non-leaf pools, and we were replacing these pools based on the total, and not the remainder (where the latter is correct)

@glemieux
Copy link
Contributor

Per the discussion at the fates software meeting last week, @rgknox is going to assess splitting this PR into two separate issues: one to remove the laimemory terms, and the other to remove the cohort level lai and sai terms. The goal in doing so would be to isolate what changes are causing non-b4b results in the SatPhen tests (recommended by @mpaiao).

@rgknox
Copy link
Contributor Author

rgknox commented Sep 13, 2022

Updated tests with the changes to fleaf.

/glade/scratch/rgknox/tests_0913-100825ch

@glemieux glemieux merged commit 2021c43 into NGEET:master Sep 14, 2022
@rgknox rgknox deleted the remove-laimemory branch October 31, 2023 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove cohort variables "lai" and "laimemory"
4 participants