Draft implementation of new OptimalChi structure

[davidorme]

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:

• completes the redefinitions of the pre-existing options for CalcOptimalChi
• restructures the rootzonestress option into PModelEnvironment for a more consistent interface and cleaner signatures.
• adds new tests to check rootzonestress handling and confirm that the new approach matches the old one.
• substitutes the new OPTIMAL_CHI_CLASS_REGISTRY interface into PModel and confirms the test suite passes with the new implementation.

The last completed changes are to:

• Remove the pmodel.calc_optimal_chi.py module
• Rename the pmodel.new_calc_optimal_chi.py objects and files
• Fix up any sphinx references
• Update the docs notebooks to remove old code and demonstrate the new implementation.

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

[codecov-commenter]

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 ⚠️
pyrealm/pmodel/pmodel.py	75.00%	2 Missing ⚠️

Additional details and impacted files

@@             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     

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

[MarionBWeinzierl]

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

[MarionBWeinzierl]

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]

@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]

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]

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 😄 I can see the value in splitting the ABC from the subclasses though.

[MarionBWeinzierl]

This looks good to me

[surbhigoel77]

The changes in the structure of OptimalChi look good to me. The changes certainly make the submodule more extendable in terms of methods and features.