giving the fraction of energy used in photosystem 2 a named constant

rgknox commented 6 months ago

Description:

Very simple, naming this constant. It really needs to not be just a 0.5 in the code, and really needs to have a name and description so people aren't confused.

Collaborators:

@rosiealice @walkeranthonyp @alistairrogers @ckoven

Expectation of Answer Changes:

None, should be B4b

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

Technical Note update:
User's Guide update:

Test Results:

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

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

FATES baseline hash-tag:

Test Output:

walkeranthonyp commented 6 months ago

Hey Ryan -- what's the nadp in the name refering to?

Nadph is produced by this process but it's a different factor for the conversion. Suggest removing the nadp part of the name.

Also in making this a parameter in the code runs the risk of someone using this to calibrate or in sensitivity analysis. This is a fixed parameter as far as I'm aware.

rosiealice commented 6 months ago

Second this – it should be fixed in the code somewhere. From: walkeranthonyp ***@***.***>Date: Monday, 6 May 2024 at 17:52To: NGEET/fates ***@***.***>Cc: Rosie Fisher ***@***.***>, Mention ***@***.***>Subject: Re: [NGEET/fates] giving the fraction of energy used in photosystem 2 a named constant (PR #1195)Hey Ryan -- what's the nadp in the name refering to?Nadph is produced by this process but it's a different factor for the conversion. Suggest removing the nadp part of the name.Also in making this a parameter in the code runs the risk of someone using this to calibrate or in sensitivity analysis. This is a fixed parameter as far as I'm aware.—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

rgknox commented 6 months ago

I pulled the nadp from a discussion in an email thread, I can remove it, I must had been confused. Does the rest of the description look ok? Feel free to propose a good description.

This is the typical convention for defining named constants with modern fortran. The "parameter" part of the declaration may be confusing because in domain sciences parameters are fluid and changeable, but in fortran this is used to declare the value immutable.

mpaiao commented 6 months ago

Thanks for this @rgknox I really like the idea of naming these terms! Perhaps we should give a name for the 4.6 term too, which converts the photosynthetically active irradiance (W/m2) to photosynthetic photon flux (µmol photons/m2/s)?

rgknox commented 6 months ago

@mpaiao and @walkeranthonyp I think I addressed your comments. @mpaiao I added that named constant and @walkeranthonyp I updated the text to remove reference to nadp. Please take a look and provide feedback.

mpaiao commented 6 months ago

It looks good with me, thanks @rgknox !

rgknox commented 6 months ago

I'm going to add some text that explains the purpose of the quadratic solve during stomatal conductance. So hold on integrating until that is in.

glemieux commented 6 months ago

Regression testing on derecho against baseline fates-sci.1.76.2_api.35.1.0-ctsm5.2.004 show all expected tests pass b4b.

location: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1195-fates

NGEET / fates