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

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

Fastslow initval #190

wants to merge 15 commits into from

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 $x_{i}$, $V_{max}$ and $J_{max}$.

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

Comment on lines +232 to 233
init_realised: tuple[NDArray, NDArray, NDArray] | None = None,
) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docstring needs updating to include this argument.

Comment on lines +327 to +328
if init_realised is not None:
self.init_xi_real, self.init_vcmax_real, self.init_jmax_real = init_realised
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to validate the provided values - mostly just that they are the correct shape. I'm not sure they need to be stored as class attributes.

Comment on lines 335 to 336
self.xi_real: NDArray = memory_effect(
self.pmodel_acclim.optchi.xi, alpha=alpha, allow_holdover=allow_holdover
Copy link
Collaborator

Choose a reason for hiding this comment

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

The init values need to be passed into memory_effect - they should replace the initial assumption that $R_{0} = O_{0}$ at $t=0$. So I think what is needed is an initial_values: NDArray | None = None argument to memory_effect and then the line memory_values[0] = values[0] can conditionally change to memory_values[0] = initial_values.

A sensible test for this would be to take the existing test and then split it into two time chunks - feeding the last realised value from the first chunk into the initial values of the second chunk should then duplicate the results of an all in one run.

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: In Progress
Development

Successfully merging this pull request may close these issues.

Allow FastSlowModel to take initial realised values
3 participants