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

new and fixed parameters for nutrients, phenology, seeds, fire and age-mortality #595

Merged
merged 30 commits into from
Jan 4, 2020

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Nov 26, 2019

Description:

This set of changes focuses only on changes to the default parameter set.

  1. New parameters were added for ECA nutrient aquisition. The code will follow, and these parameters are currently unused
  2. New parameters for non-woody stem phenology were added, this is coordinated with PR Grass phenology update #554
  3. A new parameter that flags if PFTs can experience active crown fires was added, this is coordinated with PR Active crown fire #584
  4. Parameter names for seed germination and decay, as well as the units and metadata were fixed, see parameters seed_germination_timescale and seed_decay turnover mixup? #591 and Parameter file definition edits #581

Collaborators:

@qzhu-lbl, @evaleriksen, @pollybuotte, @slevisconsulting , @jkshuman, @glemieux , @xuchongang, @ckoven @rosiealice , @JessicaNeedham

Expectation of Answer Changes:

These changes should not change answers

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

Test Results:

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

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

FATES baseline hash-tag:

Test Output:

Copy link
Contributor

@jkshuman jkshuman 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. Thanks, @rgknox

Copy link
Contributor

@glemieux glemieux left a comment

Choose a reason for hiding this comment

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

Grass phenology related updates look good to go @rgknox

@ckoven
Copy link
Contributor

ckoven commented Dec 4, 2019

Hi all,
discussed this with @rgknox and @glemieux. Main remaining question here is what to do about symbiotic N fixation parameters. Right now this PR has two dummy parameters for N fixation, i.e.: https://github.com/NGEET/fates/pull/595/files#diff-a236898dbb7848d5cfdf2610b0ba32c0R321

Is this sufficiently flexible? e.g., @rosiealice does this cover the PFT parameter needs for a FUN-like N fixation scheme? I'm imagining one PFT parameter would be a scalar variable that specifies in some units how much carbon a plant might allocate to N fixation, with zero meaning the plant isn't an N fixer. I could see this having a lower and upper bound to accommodate flexibility in representing obligate vs facultative N fixers, so therefore needing two PFT parameters?

Other PFT parameters might be the N return on the C cost, or the constants controlling the temperature sensitivity of N fixation costs, though I would expect those to be global rather than PFT parameters, and if so, maybe we can add those in once we have a fully developed N fixation model? Does anyone see a need for more than 2 PFT-level N fixation parameters now as we build out the model?

cc @walkeranthonyp @laagee

Copy link

@pollybuotte pollybuotte left a comment

Choose a reason for hiding this comment

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

One question, otherwise looks good. Sorry for the long delay in doing this!

biogeochem/EDPhysiologyMod.F90 Outdated Show resolved Hide resolved
main/EDPftvarcon.F90 Outdated Show resolved Hide resolved
@rgknox
Copy link
Contributor Author

rgknox commented Dec 13, 2019

updates: changed active_crown_fire to a scalar, and fixed the code comment noted by @pollybuotte

@jkshuman
Copy link
Contributor

@rgknox the active_crown_fire parameter will be an "on/off" switch to use the active crown fire portion of the code. It should be switched to a binary logical flag. 1=active crown fire 0=no active crown fire

@rgknox
Copy link
Contributor Author

rgknox commented Dec 16, 2019

@jkshuman , how do those changes look?

@jkshuman
Copy link
Contributor

@rgknox looks good. Thanks.

@rgknox
Copy link
Contributor Author

rgknox commented Dec 16, 2019

working with @JessicaNeedham to incorporate some of her new parameters as well, as some near-term integrations from her that use those parameters are forthcoming

Jessica Needham and others added 2 commits December 16, 2019 13:44
[Set of changes to follow with size- and age-dependent mortality
modelled as increases in mortality rate with cohort size/age. Here
just adding the parameters for these functions to the default param
file. There is a rate of change and inflection point for size and age.
Cohort age fusion parameter is also included - in age dependent
mortality we track cohort age - cohorts cannot be fused if they are
too different in age. ]

Fixes: [NGT-ED Github issue #]

User interface changes?: [Yes - users will have to specify these parameters]

Code review: [Names]

Test suite: [suite name, machine, compilers]
Test baseline:
Test namelist changes:
Test answer changes: [bit for bit, roundoff, climate changing]
Test summary: No testing.
@jkshuman
Copy link
Contributor

@rgknox I am going to pull another parameter out of the fire code. CG_strikes is a scaler on lightning datasets to identify the percentage of cloud to ground strikes. Will be easier to test and modify this if it lives in the param file as a global scaler parameter. Will take care of it this week.
https://github.com/jkshuman/fates/blob/e5e808ca3f7c4f65e4a5fd3d6dbee61a639ac9ab/fire/SFMainMod.F90#L683-L684

@rgknox
Copy link
Contributor Author

rgknox commented Dec 18, 2019

All expected tests pass, b4b. namelist tests do not pass, as expected.

/glade/scratch/rgknox/clmed-tests/fates-sci-1.31.1_api.8.1.0-Ceac01319-F26ab2875.fates.cheyenne.intel

@jkshuman, can you take a look at the new cg_strikes parameter and sign off?

@ckoven
Copy link
Contributor

ckoven commented Dec 18, 2019

I think my earlier comments to add size to a couple parameters got lost during the alphabetization step

rgknox and others added 8 commits December 18, 2019 12:01
Co-Authored-By: Charlie Koven <ckoven@users.noreply.github.com>
Co-Authored-By: Charlie Koven <ckoven@users.noreply.github.com>
Co-Authored-By: Charlie Koven <ckoven@users.noreply.github.com>
Co-Authored-By: Charlie Koven <ckoven@users.noreply.github.com>
Co-Authored-By: Charlie Koven <ckoven@users.noreply.github.com>
Co-Authored-By: Charlie Koven <ckoven@users.noreply.github.com>
Co-Authored-By: Charlie Koven <ckoven@users.noreply.github.com>
Co-Authored-By: Charlie Koven <ckoven@users.noreply.github.com>
@rgknox
Copy link
Contributor Author

rgknox commented Dec 18, 2019

see my first question in the discussion of PR 590, here: #590
It may be worth adding the namelist changes to the current PR

@rgknox
Copy link
Contributor Author

rgknox commented Jan 3, 2020

based on feedback so far on #590, I'm not going to add any changes related to hydraulics to this PR.
It looks like we will most likely make changes to the namelist variables, and how we use hlm_use_planthydro. I've mocked up some changes here: rgknox/fates@parameters-eca-phen-fire...rgknox:parameters-nlist-eca-phen-fire

I will go ahead and re-test this PR with @ckoven 's updates, this should be read to merge soon.

@rgknox
Copy link
Contributor Author

rgknox commented Jan 3, 2020

all expected pass, with NLFAILs (expected fail).
/glade/scratch/rgknox/clmed-tests/fates-sci-1.31.1_api.8.1.0-C195c9b2e-F25ff0f6e.fates.cheyenne.intel

@glemieux glemieux merged commit 17aaa87 into NGEET:master Jan 4, 2020
@jenniferholm
Copy link
Contributor

Ahh, I just missed this to add in a new parameter. My bad team. There is a fates_leaf level parameter needed for taking into account nutrients effects during photosynthesis. It's "flnr" for fraction of leaf N in Rubisco (gN Rubisco / gN leaf).
But if it's ok, I can just add this as another pull request (when I get closer to finishing the photosynthesis component), right?

@rgknox
Copy link
Contributor Author

rgknox commented Jan 5, 2020

But if it's ok, I can just add this as another pull request (when I get closer to finishing the photosynthesis component), right?

sure, we are bound to have another round of parameter updates in the near future, we just just try to clump them whenever possible

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.

8 participants