-
Notifications
You must be signed in to change notification settings - Fork 8
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
Initial draft of new quantum yield module #267
Initial draft of new quantum yield module #267
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #267 +/- ##
===========================================
- Coverage 95.29% 95.06% -0.23%
===========================================
Files 28 29 +1
Lines 1720 1802 +82
===========================================
+ Hits 1639 1713 +74
- Misses 81 89 +8 ☔ View full report in Codecov by Sentry. |
…ectlyas an argument ! Subdaily still not updated
I see you've added a csv of data related to the Sandoval k_phio approach. Over time we're slowly increasing the size of the data stored in the build data folder. Is there a case for migrating this to a Pyrealm zenodo and downloading once for each test session? This would allow us to better manage test data provenance. Alternatively, maybe we could just add a readme to the |
for more information, see https://pre-commit.ci
@j-emberton and @AmyOctoCat This is now fully implemented - could you review. We need to finalise what to do about the breaking change - we're thinking of releasing |
It's de novo generated from some appropriate inputs that are included in the folder. Having said that, at the moment we have a page in the docs about the |
OK, lets not hold up this PR with doing that but perhaps add it as an issue |
|
||
Some combinations of methods may therefore result in multiple corrections for the same | ||
limitation. At present, `pyrealm` does not automatically detect such over-correction, so | ||
take care when selecting methods for fitting the P model. |
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 think we should add this as an issue. It shouldn't be too difficult to have a compatibility check for any combination of selected methods. We could also add a quick reference to the docs for users.
If we create a v2.0 release milestone, then we can attach this issue to the v2.0 formal release and prioritise accordingly
@@ -170,6 +179,8 @@ at 25°C. This is acheived by multiplying by the reciprocal of the exponential p | |||
the Arrhenius equation ($h^{-1}$ in {cite}`mengoli:2022a`). | |||
|
|||
```{code-cell} | |||
:trusted: true | |||
|
|||
# Are these any of the existing values in the constants? |
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.
Does this comment need to be in the docs?
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've added another couple of comments but no showstoppers. I think you're good to go :)
It just occurred to me that we possibly don't want to merge into develop, as that might then constrain us from further releases under the 1.x major version. Do we instead want to create a v2.0rc branch and maintain that in parallel until v2.0 release? |
Description
This PR implements a new approach to providing quantum yield (
kphio
) values to thePModel
andSubdailyPModel
.kphio
anddo_ftemp_kphio
arguments toPModel
andSubdailyPModel
are removed. These set a referencekphio
value and then switched between constant or temperature modulated methods.method_kphio
andreference_kphio
. The method name links to a registry of subclasses of the newpyrealm.pmodel.quantum_yield.QuantumYieldABC
, which provide different methods for calculatingkphio
._calculate_kphio
abstract method, which has access to thePModelEnvironment
object for the model and the shared constants definitions, as well as a flag setting whether C3 or C4 parameterisations should be used.reference_kphio
: the meanings of this reference vary with the method, so need to be documented.In addition:
calc_modified_arrhenius_factor
implements the two different equations and this function is now used with theQuantumYieldSandoval
and replaces thecalc_ftemp_inst_vcmax
, which was basically acalc_ftemp_kphio
function is replaced byQuantumYieldTemperature
has been removed.Fixes #266
Type of change
Key checklist
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks