-
Notifications
You must be signed in to change notification settings - Fork 13
Implement compute_full_pmodel_outputs
and unit tests
#1223
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took a brief look - I'll look more at the src changes after our meeting today.
Kc = MM_Kc(Kc25, ΔHkc, T, To, R) | ||
Ko = MM_Ko(Ko25, ΔHko, T, To, R) | ||
|
||
return Kc * (1 + po2(p, oi) / Ko) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the units correct here? po2 is a Pressure, but I think Ko and Kc are in units of mol/mol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kc and Ko are Michaelis-Menten parameters that have the same units as substrate concentration (ref: Wikipedia). Here the substrates are gases, so the units of Kc and Ko can either be mol/mol (mixing ratio) or Pa (partial pressure). Since Kc and Ko are scaled via Arrhenius functions from their values at standard conditions (25 C, 1 atm), the Pa units vs. mol/mol units differ by a constant factor of 101325 Pa. The constants Kc25 and Ko25 I use are in pressure units and are the same as Stocker (2020)
I can add a comment detailing this in the code, or maybe we have a preference as to what should be standard. Currently the docstring description for the parameters is wrong, so I'll fix that.
It seems like the Farquhar code calculates everything in mol/mol, without converting to partial pressure. But as long as we are consistent, the constant factor of
(the numerator and denominator are both concentrations, so whether we use mol/mol or Pa cancels out)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with whichever approach, as long as we are consistent like you wrote, so the factor cancels out in the end, and as long as we are consistent between the two formulations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment detailing this when we declare the PModelConstants struct.
- Removed the PModelDrivers struct due to redundancy and broadcasting issues. Now we take in the drivers as separate arguments - Clarified units of Kc, Ko, and Gammastar - Removed T, P-dependent density of water. Now we only use constant density - Fixed accidental deletions during rebase
This is a duplicated PR of #1198 because git history complications made it very cumbersome to rebase. See #1198 for previous discussion. The description is copied here (and updated with latest names) for convenience:
Important note
in this PR, the PModel cannot be used yet in the canopy model because subdaily GPP variability is not explicitly separated from longer timescale acclimatized GPP. Instead
compute_full_pmodel_outputs
should be used as a ClimaLand implementation of the rpmodel package that models GPP variability assuming adjustment to optimality on weekly timescales.