Annual fAPAR_max and LAI_max

[MarionBWeinzierl]

Description

Implements the computation of fAPAR_max and LAI_max, and provides examples usages of the API (including test data).

Fixes #348 .

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: Patch coverage is 51.78571% with 54 lines in your changes missing coverage. Please review.

Project coverage is 94.75%. Comparing base (56ccf57) to head (e8d30f1).
Report is 67 commits behind head on develop.

Files with missing lines	Patch %	Lines
pyrealm/phenology/fapar_limitation.py	46.53%	54 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #403      +/-   ##
===========================================
- Coverage    96.27%   94.75%   -1.53%     
===========================================
  Files           34       38       +4     
  Lines         2659     2860     +201     
===========================================
+ Hits          2560     2710     +150     
- Misses          99      150      +51     

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

[MarionBWeinzierl]

@davidorme , could you have a look and sense-check what's there? As you suggested in #407 (I hope I understood that right) I used Scaler directly, rather than extracting any functionality.

[davidorme]

Some optimisations and a couple of issues. I think we're on the right track here. I did wonder about making the class __init__ accept daily/subdaily observations and moving the resampling in the from_Pmodel into there, but on balance I think it is right to have the straightforward usage as __init__ and then use factory methods to handle other core inputs.

[davidorme]

I think this need to check we are dealing with whole years in the input?

[MarionBWeinzierl]

Wondering whether this should be something similar as in __init__ of scaler, or something much more simple (like checking that growing_season can be divided by 365 or 366).

[davidorme]

Yeah - this was tying me in knots as well. I think it needs to be something similar to the scaler init. We want to:

• Check that the timing is regular - although that is a pain when we want to support e.g. weekly or monthly inputs
• Check that there are the same number of observations per year - although that is a pain for leap years at daily precision.

[MarionBWeinzierl]

OK, I implemented a check for the datetimes. I am sure I have missed something...

Probably a good time to implement tests.

[MarionBWeinzierl]

It currently assumes daily or subdaily inputs

[davidorme]

More thoughts - not fully formed!

[davidorme]

Yeah - this was tying me in knots as well. I think it needs to be something similar to the scaler init. We want to:

• Check that the timing is regular - although that is a pain when we want to support e.g. weekly or monthly inputs
• Check that there are the same number of observations per year - although that is a pain for leap years at daily precision.

[MarionBWeinzierl]

I have marked this as ready for review now, although there is still no integration test - I am trying to figure out whether there is something easier than the whole LAI example.

What do you think about merging in what is there and create a separate issue for the regression test @davidorme ?

[MarionBWeinzierl]

I created issue #436 to implement regression tests.

[davidorme]

@MarionBWeinzierl Some minor doc stuff and simple syntax changes. Then I think this is great as something to build regression tests and further development around.

[davidorme]

@MarionBWeinzierl I'm just wondering about what we want to do about 2.0.0 and this feature. My gut feeling is that we maintain this as a separate feature branch for now, merging #436 down into here for example, until after the 2.0.0 meta branch is merged in #383.

We don't want too many branches hanging around but it feels like it is worth keeping this partial implementation out of the 2.0.0 release candidates?

[MarionBWeinzierl]

@MarionBWeinzierl I'm just wondering about what we want to do about 2.0.0 and this feature. My gut feeling is that we maintain this as a separate feature branch for now, merging #436 down into here for example, until after the 2.0.0 meta branch is merged in #383.

We don't want too many branches hanging around but it feels like it is worth keeping this partial implementation out of the 2.0.0 release candidates?

Hm, I am not exactly enthusiastic about the idea, as it will mean more effort to make sure it does not diverge even more from the main implementation. We have merged in other not-quite-complete implementations, building things up incrementally. How is this part different?

[davidorme]

I agree it is a change in how we've handled feature branches. I want to get all of the stuff in #383 down into develop soon and that will contain breaking changes for this feature.

I guess I've also treated new features very much as a firehose of things that are immediately public, which is one reason we've ended up with a big refactor for 2.0.0. I guess I'd like 2.0.0 to be a checkpoint of what we already have to then build on?