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 cohort fusion to conserve crown area #447

Merged
merged 6 commits into from
Apr 5, 2019

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Dec 5, 2018

This PR slightly revises the cohort fusion routine, so that it conserves crown area during the fusion timestep instead of conserving total stem diameter.

Description:

Currently, when cohorts fuse, the resulting cohort tries to conserve total diameter of the two fusing cohorts. I ran into an issue where, when cohorts get really big, the numerical errors in crown area conservation get big enough to crash things. This got me thinking that the way we do cohort fusion might be leading to more numerical errors than needed, because of the lack of crown area conservation during cohort fusion, which is itself typically a part of canopy structuring. What happens in a typical canopy structure loop is that we split the cohorts between the canopy and the understory, which in general this causes some cohort to get moved between strata. But upon arrival in the new strata, the small cohort will typically find a similarly sized cohort already there, and will get fused. But because we don't conserve crown are during fusion, the total post-fusion crown area has changed, and thus the canopy structure logic isn't what it thought it was.

So what this PR does is, when fusing cohorts, to calculate the DBH of the newly fused cohort as being that DBH which conserves total crown area and the dbh-crown area allometry. further, in the event that dbh is to be incremented based on the biomass allometry to put it back on the biomass allometry, instead of immediately recalculating the dbh, it waits until the next growth step to do that. that way, if the fusion happens during the canopy structure logic, it conserves crown area during that timestep, even if doing so causes it to be above its biomass allometry, and then resets the dbh to go back onto allometry at the start of the next growth timestep.

Fixes #442.

Collaborators:

Discussions with @rgknox and @rosiealice

Expectation of Answer Changes:

This should be answer changing but at the level of noise, except for the weird edge cases that gave rise to #442. Possibly by better conserving crown area during canopy structure it might reduce radiation numerical error.

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:

Not formally tested yet. hold until tests done.

@mdietze
Copy link
Collaborator

mdietze commented Dec 5, 2018

I know I'm on the margins of FATES development, but switching fusion from conserving biomass to conserving canopy area sounds like its going to have unintended consequences for a lot of users and applications, which often are explicitly focused on carbon budgets (and thus need to conserve mass).

@ckoven
Copy link
Contributor Author

ckoven commented Dec 5, 2018

Hi @mdietze -- sorry, I was unclear. Biomass is still absolutely conserved. What is temporarily not conserved (until the subsequent timestep) is the biomass-to-dbh allometric relationship. The allometry scheme is happy for the biomass to be below allometry, but doesn't know what to do during growth if the biomass is above allometry, so when a cohort that is above its allometric biomass target tries to grow, the first step is to increment the dbh to put it back on the allometric curve. One aspect of this PR is that this putting-a-cohort-back-on-allometry step only happens during growth, not as part of the fusion process.

call StructureResetOfDH( currentCohort%prt%GetState(struct_organ,all_carbon_elements), currentCohort%pft, &
currentCohort%canopy_trim, currentCohort%dbh, currentCohort%hite )
end if
if (dbh .eq. fates_unset_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.

what is fates_unset_r8?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, i see, it means it's not set. Fair enough. Maybe add a comment so one doesn't need to go and look up the definition to understand this bit?

Copy link
Contributor

@rosiealice rosiealice left a comment

Choose a reason for hiding this comment

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

there's a comment on line 749 of EDcohortdynamics which needs changing to reflect the new logic. Otherwise I checked this and it all makes sense...

@rosiealice
Copy link
Contributor

Good to know you're keeping an eye on FATES @mdietze :)

1 similar comment
@rosiealice
Copy link
Contributor

Good to know you're keeping an eye on FATES @mdietze :)

…mization to inverse crown area allometry. Implemented safer logic against a real.
@rgknox
Copy link
Contributor

rgknox commented Dec 13, 2018

I think this looks pretty straight forward, I will issue a PR to your branch @ckoven with some edits.
Could we perform some analysis, or is there anything you can say @ckoven about how results will change in typical runs? Maybe I should compare this against a multi-decade baseline at BCI?

…ster

integrate minor updates to crown-conservation
@ckoven
Copy link
Contributor Author

ckoven commented Dec 13, 2018

@rgknox Yes, I think we should run this through the full baseline check to make sure it doesn't change things in any appreciable way.

@rgknox
Copy link
Contributor

rgknox commented Dec 14, 2018

sounds good. I will to a multi-decade run at BCI and see how things compare to master.

@walkeranthonyp
Copy link

Late to the party as usual. Just been chatting with @ckoven and @rosiealice and it seems I have a controversial suggestion for an alternative conservation scheme in the cohort fusion process. Canopy area, biomass, population weighted dbh, the canopy area to dbh allometry AND the biomass to dbh allometry can all be conserved if the number density is adjusted.

This would change the number individuals when the two cohorts are fused, which is the controversial bit, but mass, canopy area, and allometries would all be conserved. These three variables are more integrated in the mechanics of the model processes than is the number of individuals. I would expect this to make cohort fusion a smoother process.

The argument against this is that the number of individuals in a forest is measurable and so varying individual number during fusion would mess with our ability to compare against observations. I don't have a good sense of how large the change in individuals will be but I imagine that it would be small, perhaps even in the fractions of an individual which wouldn't affect our comparison to observations. In itself, that fractions of an individual can be represented suggests that we are not strict about preserving the realism of individuals.

@ckoven suggested that this would be worth testing and that he would add switch to this pull request to select alternative fusion cases. I can try to add this third case of flexible individual number.

Would be interested to hear further thoughts from those more familiar with ED: @mdietze @rgknox @tompowell9

@ckoven ckoven 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 19, 2019
@ckoven
Copy link
Contributor Author

ckoven commented Feb 19, 2019

an update on this PR: @rgknox ran tests a while ago, and it was not as close to master as we had hoped. So I've re-written the logic to allow either the old behavior (conservation of DBH) or the modified behavior (conservation of crown area) based on a switch. Currently the switch is hard-coded but maybe we should move it to the parameter file. In discussion with @walkeranthonyp last week, he had suggested a third option which he describes above, which I am happy to explore too. So, @walkeranthonyp, the code should now be ready for you to add your third option into.

I suggest we may need to investigate further here to see what the effect of the cohort fusion logic is on model behavior. I think this may call for an experiment using prescribed physiology mode, where we create a sort of benchmark case with tiny tolerances for cohort fusion and thus lots of cohorts, and then compare that to the various logic options here with more tractable cohort fusion parameters.

@walkeranthonyp
Copy link

@ckoven assignment noted. This is a good reason for me to get set up running FATES. It's prob going to take me a bit to get things working so let me know if I'm holding things up.

@ckoven
Copy link
Contributor Author

ckoven commented Feb 27, 2019

@walkeranthonyp great, thanks!

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.

From what my testing has shown, this set of changes is important in reducing error during plant promotion/demotion. I've gone through these changes in some detail myself.
Note there was a compile-time error that needed to be tracked down, but it was trivial.

@rgknox
Copy link
Contributor

rgknox commented Mar 8, 2019

Hey folks, there is a critical batch of bug-fixes coming down the pipe. These bug fixes partially require this set of changes. I'm going to set this PR to high priority to prepare for that next PR.

@rgknox
Copy link
Contributor

rgknox commented Mar 8, 2019

@ckoven , is the "not ready" label still required?

@ckoven
Copy link
Contributor Author

ckoven commented Mar 8, 2019

Hi @rgknox incorporating this PR is fine with me if it is important for overall numerical stability. We can bring @walkeranthonyp's proposed changes in as a different PR.

@ckoven ckoven 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 8, 2019
@rgknox
Copy link
Contributor

rgknox commented Mar 11, 2019

ok @ckoven . I will leave this PR up for the time being, but will likely close it, depending on how discussion of PR #501 moves along, seeing how this is a component of that PR (and that one has more extensive testing).

@rgknox rgknox merged commit 36ea2c0 into NGEET:master Apr 5, 2019
@jkshuman
Copy link
Contributor

@walkeranthonyp I was re-visiting this closed PR, and wanted to ask about your idea to alter number density in fusion and conserve other parameters. Perhaps we open a separate issue on that idea to keep it in the forefront?

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.

canopy structure crash
6 participants