Carbon allocation: bug fixes and simpler (optional) allocation approach #800
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Bug fix in DailyPRTAllometricCarbon (parteh/PRTAllometricCarbonMod.F90). When allocating to different tissues, the code was subtracting allocation of tissues before calculating the amount for the next tissue, potentially under-allocating carbon to fine roots. 2. Renamed variable laimemory with leafmemory. We were never tracking the LAI, but leaf carbon, and thus the name is dangerously misleading. 3. Implemented an option carbon allocation routine (DailyPRTAllometricCarbonSimpler in parteh/PRTAllometricCarbonMod.F90). In this routine, I applied much simpler rules for maintenance allocation: it simply checks how much each pool is in deficit, and allocates carbon (storage + carbon_balance) to the pools according to the debt. If there is any carbon left, then the plant finishes filling the storage pool and then allocates to growth.
5 tasks
…f with end select). And now all tests for hlm_parteh_mode use select case, which is safer for options in any case.
glemieux
reviewed
Jan 26, 2022
rgknox
reviewed
Feb 16, 2022
parteh/PRTAllometricCarbonMod.F90
Outdated
| ! IV. If carbon balance is negative, reduce the storage pool. Otherwise, try to | ||
| ! fill the storage pool before growing. | ||
| ! ----------------------------------------------------------------------------------- | ||
| update_storage: if ( carbon_balance < 0.0_r8 .or. is_hydecid_dormant ) then |
rgknox
reviewed
Feb 16, 2022
rgknox
reviewed
Feb 16, 2022
…streamline the code. 1. Removed some of the excessively cautious checks on carbon balance and storage for drought deciduous. Instead, the code now prevents growth by setting the target storage to include any positive carbon balance (currently unlikely, but will be common with semi-deciduous plants). 2. Streamline some of the allocation steps, by using an allocation factor that is shared across all tissues. This is mathematically equivalent to the previous code, but a bit safer. 3. Included some detailed checks on allometry, with longer reports in case the model fails to converge, which can be useful for debugging.
rgknox
reviewed
Feb 28, 2022
rgknox
reviewed
Mar 7, 2022
rgknox
reviewed
Mar 7, 2022
main/FatesHistoryInterfaceMod.F90
Outdated
| @@ -4069,7 +4079,7 @@ subroutine update_history_hydraulics(this,nc,nsites,sites,bc_in,dt_tstep) | |||
|
|
|||
| jsoil = jrhiz + jr1-1 | |||
| vwc = bc_in(s)%h2o_liqvol_sl(jsoil) | |||
| psi = site_hydr%wrf_soil(jrhiz)%p%psi_from_th(vwc) | |||
| psi = site_hydr%wrf_soil(jrhiz)%p%psi_from_th(vwc) ! MLO: Any reason for not using smp_sl? | |||
There was a problem hiding this comment.
yeah, not sure why we are using this method, it does seem circuitous
rgknox
reviewed
Mar 7, 2022
|
Tests on cheyenne are expected, but the bug fix does change results: /glade/scratch/rgknox/tests_0308-101142ch Also running a multi-decade smoke test on the f45. will report when completed |
|
@rgknox Thanks for testing. This fix certainly changes the results because it effectively increases allocation to fine roots. |
|
I merged in the bug fix PR sci1.55.3, and ran a multi decade simulation on the f45 grid. The simulation is crashing after 15 years, 5 months. Still investigating, but here is the traceback and relevant lines: |
This was referenced Mar 10, 2022
Open
|
@mpaiao I created another branch that just had the re-ordering fix in the existing allocation scheme: master...rgknox:sci.1.55.4_api.22.1.0-calloc-onlyfix My intention here is to run a comparison simulation between this new branch and your branch to see if they generate indistinguishable results with a default setup on a 10 year global simulation. If they do, then we can be assured that the changes to results from your branch are sourced exactly from the code we think it is. Can you take a look at that branch and tell me if this all makes sense? |
|
@rgknox I checked the branch and it looks fine, thanks! |
Removed the if statements for hydro-deciduous when dormant. These changes will be included in the drought deciduous pull request.
Replaced 1 with "itrue" in some logical tests.
The target leaf carbon should be zero when leaves are off. This was accidentally removed when deleting some code that should be incorporated when pull request NGEET#801 (Multiple updates to drought deciduous phenology) is incorporated.
…y (another PR) and to reduce differences between branch and master.
…on. This requires a change in the host land models.
…is not related to carbon allocation but semi-deciduous leaf phenology.
mpaiao
commented
Apr 25, 2022
There was a problem hiding this comment.
@rgknox I went through every file once again, and I cannot find any change besides the carbon allocation that would explain the changes. I made a few minor edits to reconcile the PR with the main line, but they shouldn't have any effect on results.
If you prefer, I could withdraw this PR and re-submit the carbon allocation and the other minor changes as multiple pull requests, let me know what you think.
rgknox
added
the
PR status: Not Ready
mpaiao
added a commit
to mpaiao/fates
that referenced
this pull request
May 3, 2022
…ots and leaves are truly proportional to demand. Description: 1. Bug fix in DailyPRTAllometricCarbon (parteh/PRTAllometricCarbonMod.F90). When allocating to different tissues, the code was subtracting allocation of tissues before calculating the amount for the next tissue, potentially under-allocating carbon to fine roots. 2. Minor code updates to simplify allocation calculations. It now uses a single allocation factor based on availability and total need, which is applied to all tissues. 3. Replaced a few if statements with select case, which simplifies adding other hypotheses in the future (and it's safer for selecting cases). This pull request addresses the bug discussed in NGEET#784 and supersedes pull request NGEET#800. Additional minor changes in former PR NGEET#800 will be added as subsequent pull requests.
This was referenced May 4, 2022
mpaiao
added a commit
to mpaiao/fates
that referenced
this pull request
May 4, 2022
Implemented an option carbon allocation routine (DailyPRTAllometricCarbonSimpler in parteh/PRTAllometricCarbonMod.F90). In this routine, I applied much simpler rules for maintenance allocation: it simply checks how much each pool is in deficit, and allocates carbon (storage + carbon_balance) to the pools according to the debt. If there is any carbon left, then the plant finishes filling the storage pool and then allocates to growth. This option is set by fates_parteh_mode=0, and will require changing the valid options in the host land models (see example of changes for CTSM in here). This was extracted from the now defunct PR NGEET#800. Kept as a separate pull request if we ever want to bring it to the mainline.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request addresses the bug discussed in #784. I changed the order of the carbon allocation fraction, to ensure that allocation to fine roots and leaves were truly proportional to demand.
Description:
fates_parteh_mode=0, and will require changing the valid options in the host land models (see example of changes for CTSM in here).This addresses issue #784
Collaborators:
@rgknox @jkshuman
Expectation of Answer Changes:
This fixes the carbon allocation priorities, so it should cause higher use of carbon balance and storage than the current master.
Checklist:
Test Results:
CTSM (or) E3SM (specify which) test hash-tag:
CTSM (or) E3SM (specify which) baseline hash-tag:
FATES baseline hash-tag:
Test Output: