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

Aligning PModel and Subdaily PModel #401

Open
wants to merge 17 commits into
base: 383-meta-release-200
Choose a base branch
from

Conversation

davidorme
Copy link
Collaborator

@davidorme davidorme commented Jan 27, 2025

Description

This is a draft of a new structure for the PModel and SubdailyPModel:

  • It centralises a lot of the shared attributes and docstrings in the PModelABC, which includes a single abstract class _fit_model.
  • Each of the PModel and SubdailyPModel subclasses then:
    • Defines the specific _fit_model for the class.
    • Defines an __init__ method that:
      • calls super.__init__(...) to initialise the superclass attributes
      • adds any specific attributes for the subclass, and
      • lastly, calls self._fit_model()

The result aligns the internal attribute names and shares a lot of docstrings and boilerplate. It also dumps the separate PModel.estimate_productivity step and fapar and ppfd are now taken directly from the PModelEnvronment (see PR #390 - currently still open, but nearly in, I think).

  • The old implementation has been replaced in tests and all tests pass (except in one currently skipped test, which hasn't worked since the early days - we should delete it).

Things still to do but probably not in this PR.

  • Strip out the old implementations - this PR is already bulky, with more changes coming so I would prefer to leave them in place - they are still used in the docs currently, until more of the 2.0.0 API changes have merged and then remove them in one go.

  • Not all attributes defined on each model are populated. I need to chase down the science on these and update them, but I think for the moment I will just leave them as unpopulated.

  • It would be nice to use @cachedproperty on some of these attributes to keep overheads down. For example, PModel.rd might only be calculated on request. This seems like a separate PR, and shouldn't break the API, so can be post 2.0.0.

Fixes #386
Fixes #385

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

@j-emberton
Copy link
Collaborator

Hi @davidorme.

Just taken a look at this. Overall I think it looks really good and is a big improvement on what we had before.

Areas for improvement:

  • I feel the constructor of the PModelABC base class is a bit cluttered and contains a lot of similar repeated operations when evaluating and retrieving different methods and models. There's potential to rationalise these operations into class methods and make them more testable. But this isn't a showstopper.
  • Do we want to add explicit unit checking? This could be as simple as checking feasible physical values for inputs such as ppfd or something more complex such as using Pint to enforce unit consistency when writing a longer analysis script that uses multiple Pyrealm classes.

@davidorme
Copy link
Collaborator Author

Areas for improvement:

  • I feel the constructor of the PModelABC base class is a bit cluttered and contains a lot of similar repeated operations when evaluating and retrieving different methods and models. There's potential to rationalise these operations into class methods and make them more testable. But this isn't a showstopper.

Yup - I agree that could be packaged more cleanly. I'll have a look at this but might park some of this for later polishing. I think the big picture shape of the new API and keeping the tests working is key on this PR.

  • Do we want to add explicit unit checking? This could be as simple as checking feasible physical values for inputs such as ppfd or something more complex such as using Pint to enforce unit consistency when writing a longer analysis script that uses multiple Pyrealm classes.

On the bounds checking - yes, but not here. Once this is done, PPFD and FAPAR will move into PModelEnvironment, and that adds bounds checks already (no issue yet, but see #394)

@j-emberton
Copy link
Collaborator

Hi @davidorme , is this ready to go now?

@davidorme
Copy link
Collaborator Author

@j-emberton Nope - this is going to sprawl a lot more, but there are a couple of incoming PRs that will make this switch cleaner.

@j-emberton
Copy link
Collaborator

@j-emberton Nope - this is going to sprawl a lot more, but there are a couple of incoming PRs that will make this switch cleaner.

ah cool. I thought I saw a refreshed request for review come in so thought I'd double check.

@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 94.21488% with 14 lines in your changes missing coverage. Please review.

Project coverage is 93.35%. Comparing base (56ccf57) to head (509b279).
Report is 56 commits behind head on develop.

Files with missing lines Patch % Lines
pyrealm/pmodel/new_pmodel.py 96.61% 7 Missing ⚠️
pyrealm/pmodel/pmodel_environment.py 74.07% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #401      +/-   ##
===========================================
- Coverage    96.27%   93.35%   -2.92%     
===========================================
  Files           34       37       +3     
  Lines         2659     2951     +292     
===========================================
+ Hits          2560     2755     +195     
- Misses          99      196      +97     

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

@davidorme
Copy link
Collaborator Author

@j-emberton - Just to refresh on these two questions:

  • I feel the constructor of the PModelABC base class is a bit cluttered and contains a lot of similar repeated operations when evaluating and retrieving different methods and models. There's potential to rationalise these operations into class methods and make them more testable. But this isn't a showstopper.

I've done that - it doesn't really save a lot of lines but the structure and the separation of attribute typing and population does look cleaner to me.

  • Do we want to add explicit unit checking? This could be as simple as checking feasible physical values for inputs such as ppfd or something more complex such as using Pint to enforce unit consistency when writing a longer analysis script that uses multiple Pyrealm classes.

All the forcing variables inputs are now via PModelEnvironment (pending merge of #390) and are now checked via the new BoundsChecker class (merged from #409)

@davidorme davidorme marked this pull request as ready for review February 7, 2025 09:12
j-emberton
j-emberton previously approved these changes Feb 7, 2025
Copy link
Collaborator

@j-emberton j-emberton left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @davidorme . This looks good

@j-emberton j-emberton self-requested a review February 7, 2025 09:31
@j-emberton j-emberton dismissed their stale review February 7, 2025 09:32

Wait till PR#390 has been merged

@j-emberton
Copy link
Collaborator

Sorry. I got ahead of myself. Have dismissed my approval

@davidorme davidorme changed the base branch from develop to 383-meta-release-200 February 7, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review
3 participants