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

[Meta] Improve API and functionality for restarting subdaily models #333

Open
2 tasks
davidorme opened this issue Oct 18, 2024 · 0 comments
Open
2 tasks
Labels
enhancement New feature or request icebox Issues that are not currently a priority for development meta Issues that act as the central point of reference for a larger piece of work

Comments

@davidorme
Copy link
Collaborator

davidorme commented Oct 18, 2024

In #190, we implemented a way of "restarting" a SubdailyPModel. This allows the internal interpolation of realised values for xi, vcmax25 and jmax25 to pick up where a previous contiguous time period ended and avoid:

  1. the assumption that the initial realised values are the observed values.
  2. the gap in prediction on the first day before the first acclimation window is reached.

The code in PR #190 implements this by allowing previous realised values to be passed in to restart calculations but:

  • It has a rather unfriendly API (provide values as a tuple of arrays manually extracted from the previous model). A better API would be to accept some kind of a class instance as an input and do the extractions internally. This could simply be a whole SubdailyPModel instance but those can be quite bulky if there is a large number of observations or datetimes. Better would be to have a SubdailyPModel.get_restart method that creates a minimal RestartSubdailyPModel dataclass with some de/serialisation options for saving to disk and passing around.
  • It only implements it for the previous interpolation method and not for the linear interpolation. The interpolation takes place in the SubdailyScaler.fill_daily_to_subdaily method. Without previous values, there is a gap filled with np.nan for the observations that fall on the first day but at times before the first acclimation window. Those need to be filled from previous values to make the values using previous_value identical to simply merging the two contiguous time blocks and running in one go.
    • It's easy for the previous method as this is just duplicating the exact same value at the start of the interpolation
    • It's not as easy for linear. In addition to the previous realised values, the fill_daily_to_subdaily needs the last set of daily acclimation values as there is an offset in acclimation to avoid plants "knowing" where they are acclimating towards.

However - this is quite a lot of effort and I think we should wait for user demand before committing more time to this functionality. We have a proof of concept and this issue is mostly to record what the next steps might be if that demand materialises. At that point we can convert those tick boxes to issues.

@davidorme davidorme added the meta Issues that act as the central point of reference for a larger piece of work label Oct 18, 2024
@davidorme davidorme added the enhancement New feature or request label Oct 18, 2024
@davidorme davidorme added the icebox Issues that are not currently a priority for development label Oct 18, 2024
@davidorme davidorme mentioned this issue Oct 18, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request icebox Issues that are not currently a priority for development meta Issues that act as the central point of reference for a larger piece of work
Projects
Status: Icebox
Development

No branches or pull requests

1 participant