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

Fastslow initval #190

Merged
merged 43 commits into from
Oct 21, 2024
Merged

Fastslow initval #190

merged 43 commits into from
Oct 21, 2024

Conversation

surbhigoel77
Copy link
Collaborator

@surbhigoel77 surbhigoel77 commented Mar 6, 2024

Description

The subdaily module uses an extended form of PModel called the FastSlowPModel. to estimate two categories of variables - fast variables (that respond to environmental changes instantaneously) and slow variables (that take time to respond to these changes). The module uses a memory_effect functions that facilitates the calculation of the slow variables as a combination of their lag values and optimal values.

In the current implementation of subdaily, at the start of the model run, the slow variables take the optimal value as the initial value and update the succeeding values using the:

$R_{t} = R_{t-1}(1 - \alpha) + O_{t} \alpha$

This PR intends to use an approach in which the last available realised value is used as the initial value for $\xi$, $V_{max}$ and $J_{max}$.

[ETA - @davidorme 2024-10-18]

In addition to adjusting the equation above (so that $R_1$ can be updated from the provided previous values $R_0$ and not just be set as $O_1$), the previous values also need to be supplied to SubdailyScaler.fill_daily_to_subdaily to replace the leading np.nan values before the first acclimation window.

Fixes #82

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

@davidorme
Copy link
Collaborator

This is a WIP, @surbhigoel77 ? Can you convert to a draft (link in Reviewers pane).

@davidorme
Copy link
Collaborator

Also - have you branched this off the feature/new-unit-tests branch, rather than develop? I think it is bringing the commits to that PR with it?

@surbhigoel77 surbhigoel77 marked this pull request as draft March 6, 2024 14:53
@surbhigoel77
Copy link
Collaborator Author

Also - have you branched this off the feature/new-unit-tests branch, rather than develop? I think it is bringing the commits to that PR with it?

Yes, I realised that when I pushed changes to fastslow-initval. The commit history shows changes from feature/new-unit-tests branch as well.

@surbhigoel77 surbhigoel77 self-assigned this Mar 6, 2024
@davidorme
Copy link
Collaborator

No bother - it isn't a big deal 😄 I guess we could git revert those commits on this branch

@MarionBWeinzierl MarionBWeinzierl added enhancement New feature or request rse labels Mar 11, 2024
@surbhigoel77 surbhigoel77 requested a review from davidorme May 8, 2024 21:20
@MarionBWeinzierl MarionBWeinzierl marked this pull request as ready for review May 14, 2024 13:00
Copy link
Collaborator

@davidorme davidorme left a comment

Choose a reason for hiding this comment

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

Definitely the right inputs, but then we need to feed those forward into the calculations. This is a little more finicky than I first thought - I'm going to take it off the list for version 1.0.0 (#206)

pyrealm/pmodel/subdaily.py Outdated Show resolved Hide resolved
pyrealm/pmodel/subdaily.py Outdated Show resolved Hide resolved
pyrealm/pmodel/subdaily.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.85%. Comparing base (1f315ba) to head (b114852).
Report is 216 commits behind head on develop.

Files with missing lines Patch % Lines
pyrealm/pmodel/subdaily.py 86.66% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #190      +/-   ##
===========================================
+ Coverage    95.29%   95.85%   +0.56%     
===========================================
  Files           28       34       +6     
  Lines         1720     2487     +767     
===========================================
+ Hits          1639     2384     +745     
- Misses          81      103      +22     

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

@MarionBWeinzierl MarionBWeinzierl self-assigned this Oct 2, 2024
@davidorme
Copy link
Collaborator

@MarionBWeinzierl It looks good - I've added a higher level test to run it through the whole SubdailyPModel process. That revealed a couple of minor issues in the value checking. I've also updated the variable name and docstrings in SubdailyPModel.

Unfortunately, the new test is failing for reasons I don't quite understand yet.

@davidorme
Copy link
Collaborator

davidorme commented Oct 18, 2024

OK.

  • I figured out the problem - need to fill the values back to subdaily observations as well - and fixed it for the specific case of using fill_kind = "previous".
  • I've updated the original PR description to describe this extra work.
  • I've raised a new meta issue [Meta] Improve API and functionality for restarting subdaily models #333 to put a pin in where we are and where we might want to pick up if user demand appears.

@MarionBWeinzierl MarionBWeinzierl merged commit 2883d59 into develop Oct 21, 2024
12 checks passed
@MarionBWeinzierl MarionBWeinzierl deleted the fastslow-initval branch October 21, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rse
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow FastSlowModel to take initial realised values
5 participants