Closed davidorme closed 1 week ago
Attention: Patch coverage is 92.68293%
with 12 lines
in your changes missing coverage. Please review.
Project coverage is 95.06%. Comparing base (
1f315ba
) to head (16b45aa
). Report is 5 commits behind head on develop.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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 pyrealm_build_data/sandoval_k_phio
folder to note where this data is sourced from for any future developers.
@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 2.0.0rc1
to give us some wiggle room on making the functionality available but not committing to the final state of a new 2.0.0
API.
Alternatively, maybe we could just add a readme to the
pyrealm_build_data/sandoval_k_phio
folder to note where this data is sourced from for any future developers.
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 pyrealm_build_data
package but that's separated from the package contents. It would actually make a lot more sense to move the documentation in there into docstrings in __init__.py
files within the pyrealm_build_data
package modules and then use autodoc
to maintain an API like documentation tree for the package.
Alternatively, maybe we could just add a readme to the
pyrealm_build_data/sandoval_k_phio
folder to note where this data is sourced from for any future developers.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
pyrealm_build_data
package but that's separated from the package contents. It would actually make a lot more sense to move the documentation in there into docstrings in__init__.py
files within thepyrealm_build_data
package modules and then useautodoc
to maintain an API like documentation tree for the package.
OK, lets not hold up this PR with doing that but perhaps add it as an issue
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 a $V_{cmax}$ specific version of the function.calc_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