Skip to content

Add cyclic forcing support#2081

Merged
SouthEndMusic merged 11 commits into
mainfrom
cyclic_forcing
Feb 25, 2025
Merged

Add cyclic forcing support#2081
SouthEndMusic merged 11 commits into
mainfrom
cyclic_forcing

Conversation

@SouthEndMusic

@SouthEndMusic SouthEndMusic commented Feb 20, 2025

Copy link
Copy Markdown
Collaborator

Fixes #153.

Notes:

  • With timeseries which are extrapolated in a periodic manner, the first and last data values must be validated to be the same. For linear interpolations this ensures continuity, and for constant interpolation this value would be otherwise ignored.
  • Time periods like years and months do not always have the same length, so this can get out of sync when these time periods are repeated. This should at least be documented well, and might need a follow-up issue.
  • We add all time values in the input to the tstops. With cycling forcing this has to be expanded to all time values in all periods that fall within the simulation period.

@SouthEndMusic SouthEndMusic marked this pull request as draft February 20, 2025 12:15
@SouthEndMusic SouthEndMusic changed the title Add test model with cyclic forcing Add cyclic forcing support Feb 20, 2025
@SouthEndMusic SouthEndMusic marked this pull request as ready for review February 24, 2025 13:56
@SouthEndMusic SouthEndMusic requested a review from visr February 24, 2025 13:56
Comment thread docs/reference/usage.qmd Outdated
Comment on lines +180 to +182
## cyclic time series

When cyclic forcing is set to true for a node in the Node table, every time series associated with that node in the corresponding table(s) will be interpreted as cyclic. That is: the time series is exactly repeated left and right of the original time interval to cover the whole simulation period. For this it is validated that the first and last data values in the timeseries are the same. For instance, quarterly precipitation requires giving values for every quarter at the start of the quarter, and then the value for the first quarter again at the start of the next year.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
## cyclic time series
When cyclic forcing is set to true for a node in the Node table, every time series associated with that node in the corresponding table(s) will be interpreted as cyclic. That is: the time series is exactly repeated left and right of the original time interval to cover the whole simulation period. For this it is validated that the first and last data values in the timeseries are the same. For instance, quarterly precipitation requires giving values for every quarter at the start of the quarter, and then the value for the first quarter again at the start of the next year.
## Cyclic time series
When `cyclic_time` is set to true for a node in the Node table, every time series associated with that node in the corresponding table(s) will be interpreted as cyclic. That is: the time series is exactly repeated left and right of the original time interval to cover the whole simulation period. The first and last data values in the timeseries need to be the same. For instance, quarterly precipitation requires giving values for every quarter at the start of the quarter, and then the value for the first quarter again at the start of the next year.

@visr

visr commented Feb 25, 2025

Copy link
Copy Markdown
Member

Looks good, but there are still some tests to fix it seems:

FAILED python\ribasim\tests\test_io.py::test_sort - AssertionError: assert ['node_id', '...condition_id'] == ['node_id', '...greater_than']
  
  At index 2 diff: 'condition_id' != 'greater_than'
  
  Full diff:
    [
        'node_id',
        'compound_variable_id',
  -     'greater_than',
  +     'condition_id',
    ]
= 1 failed, 53 passed, 1 skipped, 1 xfailed, 1 xpassed, 30 warnings in 58.63s =

Does this need to merge master?

@SouthEndMusic

Copy link
Copy Markdown
Collaborator Author

@visr this is because I did a small follow-up of #2079 where the sorting of the DiscreteControl / condition table was changed, and apparently we have 2 sources of truth for those (one in Python and one in Julia). I wasn't aware this was part of a test, will quickly fix.

@SouthEndMusic SouthEndMusic merged commit bb62a1a into main Feb 25, 2025
@SouthEndMusic SouthEndMusic deleted the cyclic_forcing branch February 25, 2025 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cyclic forcing

2 participants