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

Implementation of Arrhenius methods #389

Merged
merged 24 commits into from
Jan 24, 2025
Merged

Implementation of Arrhenius methods #389

merged 24 commits into from
Jan 24, 2025

Conversation

davidorme
Copy link
Collaborator

@davidorme davidorme commented Jan 18, 2025

Description

This PR fixes #384, but also goes quite a bit further in trying to future proof the API of thePModel and SubdailyPModel a bit. The basic story is:

  • The PModel currently follows rpmodel in using a modified Arrhenius equation parameterised by Kattge and Knorr (2007). This is inconsistent with SubdailyPModel, is implemented a bit oddly (t_growth should not be the same as the observed temperature) and there's no general agreement that using this form is better than the simple form at present.

  • So, the simple equation (currently in calc_ftemp_arrh) should be used but it is highly likely that we're going to want to be alter the Arrhenius scaling in the near future.

So this PR:

  • Renames the existing Arrhenius scaling functions (calc_ftemp_arrh -> calculate_simple_arrhenius_factor and calc_modified_arrhenius_factor -> calculate_kattge_knorr_arrhenius_factor) and tidies the function implementation up a bit.
  • Adds a new pyrealm.pmodel.arrhenius module that implements the ArrheniusFactorABC and
    ARRHENIUS_METHOD_REGISTRY. This ABC is really just a wrapper around the standalone functions that is tailored to run the calculations from within the context of a PModel or SubdailyPModel environment and the registry allows the implementations to be accessed via a short method name.
  • Implemented Subclasses set a class method attribute, which defines that short name. This name is used to:
    • automatically register the subclass using __init_subclass__,
    • expose that method option through the registry via the new method_arrhenius argument to PModel, and
    • to look up the required coefficients in the PModelConsts.
  • When an instance is created, it sets up commonly used variable (tk and tkref) and then the calculate_arrhenius_factor method can be called with the coefficients for a given enzyme system. The base method checks the coefficients automatically and then calls the private _calculation_method abstract method defined by the subclass.

The PR then also:

  • Fixes up the tests for this and adds some simple testing of the new ABC.
  • Adds a brand new test that checks that the PModel and SubdailyPModel give the same outputs for fixed data. Previously, they did not.
  • Removes the recent implementation of the different modes for the Kattge Knorr Arrhenius scaling. The suggested use of "J1942" over "M2002" turns out to have been a bit of a storm in a teacup and there is general agreement that "M2002" should always be used.

I think the altered PModel API is sensible - even if the internals change a bit, the signatures should be stable.

Fixes #384

Type of change

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist

  • Make sure you've run the pre-commit checks: $ pre-commit run -a
  • All tests pass: $ poetry run pytest

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

Comment on lines 120 to 146
# Arrhenius values
arrhenius_vcmax: dict = field(
default_factory=lambda: dict(
simple=dict(ha=65330),
kattge_knorr=dict(
entropy_intercept=668.39,
entropy_slope=-1.07,
ha=71513,
hd=200000,
),
)
)
"""Coefficients of Arrhenius factor scaling for :math:`V_{cmax}`."""

arrhenius_jmax: dict = field(
default_factory=lambda: dict(
simple=dict(ha=43900),
kattge_knorr=dict(
entropy_intercept=659.70,
entropy_slope=-0.75,
ha=49884,
hd=200000,
),
)
)
"""Coefficients of Arrhenius factor scaling for :math:`J_{max}`."""

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am sorry to be a bit stupid (Python is not my first language), but I am confused about this. Can you explain to me what the field in all those dicts and lambdas does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a peculiarity of the dataclass API. Default dataclass attributes get created from the class definition and so the value of mutable defaults ends up being shared and edited across all instances of the dataclass. So it forbids mutable defaults using the simple attr: type = val system and instead you have to wrap the default value in a lambda through the field constructor, which gives finer control over the attribute.

https://docs.python.org/3/library/dataclasses.html#mutable-default-values

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah. Oh dear. 😆

@MarionBWeinzierl
Copy link
Collaborator

But... right now does this seem like a sensible approach?

From what I understand this looks sensible, and much cleaner.

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 97.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.34%. Comparing base (56ccf57) to head (3ea00ba).
Report is 11 commits behind head on develop.

Files with missing lines Patch % Lines
pyrealm/pmodel/arrhenius.py 98.14% 1 Missing ⚠️
pyrealm/pmodel/pmodel.py 93.75% 1 Missing ⚠️
pyrealm/pmodel/subdaily.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #389      +/-   ##
===========================================
+ Coverage    96.27%   96.34%   +0.06%     
===========================================
  Files           34       35       +1     
  Lines         2659     2737      +78     
===========================================
+ Hits          2560     2637      +77     
- Misses          99      100       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidorme davidorme marked this pull request as ready for review January 22, 2025 18:10
Copy link
Collaborator

@MarionBWeinzierl MarionBWeinzierl left a comment

Choose a reason for hiding this comment

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

This looks good to me, just one minor complaint about an outdated comment.

@davidorme davidorme merged commit df515aa into develop Jan 24, 2025
13 checks passed
@davidorme davidorme deleted the 384-simple-arrhenius branch January 24, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Change the calculation of Jmax25 and Vcmax25 in PModel to the simple form
3 participants