ImperialCollegeLondon / pyrealm

Development of the pyrealm package, providing an integrated toolbox for modelling plant productivity, growth and demography using Python.
https://pyrealm.readthedocs.io/
MIT License
19 stars 8 forks source link

Draft implementation of new OptimalChi structure #172

Closed davidorme closed 7 months ago

davidorme commented 7 months ago

Description

This is PR implements the first stage of addressing #119, allowing the main PModel code to be used within subdaily caculations. These changes provide a complete refactor of the CalcOptimalChiclass, which has a bunch of different approaches as methods, into the NewCalcOptimalChi abstract base class (ABC), which then set of subclasses providing the different approaches.

The main aims here are:

  1. Provide a way to recalculate chi using modified values of the xi variable. The ABC requires subclasses to provide set_beta and estimate_chi: both of these methods are used as part of __init__ for a derived class, but estimate_chi can also be used separately to inject new xi values.
  2. Make it easier and cleaner to add new approaches - with the new setup, users can actually create their own subclasses, should they want to. For this reason, a registry object is added that allows specific approaches to be retrieved using a method name.

One thing we might do in future but seems paranoid is make the method and is_c4 class attributes read only, but class attributes in ABCs seem finicky: using @property seems like one approach but then causes mypy issues with setting the values. That is paranoid though.

The PR now:

The last completed changes are to:

Type of change

Key checklist

Further checks

codecov-commenter commented 7 months ago

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (82565ca) 93.83% compared to head (b3a03ca) 93.48%. Report is 5 commits behind head on develop.

Files Patch % Lines
pyrealm/pmodel/optimal_chi.py 87.74% 19 Missing :warning:
pyrealm/pmodel/pmodel.py 75.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #172 +/- ## =========================================== - Coverage 93.83% 93.48% -0.36% =========================================== Files 28 28 Lines 1395 1458 +63 =========================================== + Hits 1309 1363 +54 - Misses 86 95 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MarionBWeinzierl commented 7 months ago

@davidorme , would Final attributes (see here) do what you want, instead of using @property ?

MarionBWeinzierl commented 7 months ago

Also, and this might be controversial and/or philosophical: I would put the abstract base class and each derivative in its own file, because otherwise the file might become large, and if people derive their own subclasses they might want to keep that with their module, and not all add it into this one file.

If, on the other hand, you believe there won't be any additional implementations of that abstract class it is fine to keep everything together in one file.

davidorme commented 7 months ago

@davidorme , would Final attributes (see here) do what you want, instead of using @property ?

That's interesting. I'm a bit conflicted about Final because it seems to step over a line between indicating typing and controlling the usage of objects - I think I've just got a narrow view on what mypy does though.

More broadly, I should have deleted those @property comments. I've used that approach before but it is really defensive and adds complexity - if people manually change the properties of class attributes, then they should expect to get bitten 😄 I've always tended to be that defensive, but I've learned that this is often at the expense of making the code less transparent.

davidorme commented 7 months ago

if people derive their own subclasses they might want to keep that with their module, and not all add it into this one file.

My intention in the implementation here is that someone can include a new subclass outside of the package code. If they have a python file that does this:

from pyrealm.pmodel.calc_optimal_chi_new import CalcOptimalChiNew

class MyCustomCalcOptChi(CalcOptimalChiNew, method = 'custom_opt_chi', is_c4=False):
    ...

then the __init_subclass__ would add the method to the registry and they could then use PModel(..., method='custom_opt_chi). Once, that is, this approach is rolled out so that PModel does something like:

try:
    opt_chi_method = OPTIMAL_CHI_CLASS_REGISTRY[method]
except KeyError:
    ...

self.opt_chi = opt_chi_method(...)
davidorme commented 7 months ago

f people derive their own subclasses they might want to keep that with their module

Of course if people wanted to contribute that method to the package, then this could get out of hand, but that seems a way away :smile: I can see the value in splitting the ABC from the subclasses though.