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

Support conditions on linear combinations of variables for DiscreteControl #1371

Merged
merged 33 commits into from
Apr 16, 2024

Conversation

SouthEndMusic
Copy link
Collaborator

@SouthEndMusic SouthEndMusic commented Apr 10, 2024

Fixes #1346.

Upgrade instructions

This is a breaking change to the DiscreteControl tables, that makes it possible to listen to a weighted combination of multiple variables. Until so far we were only able to listen to one variable, like the water level in a particular Basin.

To listen to the average of four water levels, you can assign a weight of 0.25 to each. To listen to the difference of two water levels, you can assign a weight of 1 to one, and -1 to the other.

For a description of the new table schemas, see https://deltares.github.io/Ribasim/core/usage.html#sec-discrete-control. The combinations of variables are called compound variables, and are identified by a shared compound_variable_id.

@SouthEndMusic SouthEndMusic marked this pull request as draft April 10, 2024 11:43
@SouthEndMusic
Copy link
Collaborator Author

I Had some problems with the table name CompoundVariable, apparently this is not allowed since the schema name which contains the snake case compound_variable may not contain underscores.

@SouthEndMusic SouthEndMusic changed the title Python side Support conditions on linear combinations of variables for DiscreteControl Apr 10, 2024
core/src/callback.jl Outdated Show resolved Hide resolved
@SouthEndMusic SouthEndMusic marked this pull request as ready for review April 11, 2024 07:00
@SouthEndMusic SouthEndMusic requested a review from visr April 11, 2024 07:00
@SouthEndMusic SouthEndMusic requested review from evetion and removed request for visr April 15, 2024 09:01
Copy link
Member

@evetion evetion left a comment

Choose a reason for hiding this comment

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

Nice PR! 👍🏻 I wonder if we should change the input tables slightly (but it would be breaking).

Specifically, compoundvariable is now mostly a repeat of condition, with the change of only a single field (added weight, dropped greater_than). Furthermore, condition is changed that type can take "compound", while the node_id is ignored.

I wonder whether we should split condition into condition and variable, where variable can be compound if a id occurs multiple times. listen-*, variable and look ahead are then all dropped/moved.

core/test/control_test.jl Show resolved Hide resolved
core/src/validation.jl Outdated Show resolved Hide resolved
@SouthEndMusic
Copy link
Collaborator Author

Some clarifications for the latest commits:

  • We're now back at allowing multiple compound variables on the same DiscreteControl node using the compound_variable_id. This means that we're back in the situation where all the control of a whole model can be done with a single DiscreteControl node, so it's up to the modeller to do this in a sensible way;
  • We also allow multiple greater_than values on a single compound variable, which all define their own condition. I did not want to copy the compound variable data for every condition, so I now store all greater_than values per compound variable in discrete_control.greater_than. This does require some translation between the condition_idx (which is for a single flat dense vector) and the compound variable and greater_than indices, see get_discrete_control_indices.

@SouthEndMusic SouthEndMusic requested a review from evetion April 16, 2024 11:29
@SouthEndMusic SouthEndMusic added the breaking A change that breaks existing models label Apr 16, 2024
docs/core/usage.qmd Outdated Show resolved Hide resolved
docs/core/usage.qmd Outdated Show resolved Hide resolved
core/src/util.jl Show resolved Hide resolved
docs/core/usage.qmd Show resolved Hide resolved
python/ribasim/ribasim/model.py Outdated Show resolved Hide resolved
@SouthEndMusic SouthEndMusic merged commit f6108a0 into main Apr 16, 2024
24 checks passed
@SouthEndMusic SouthEndMusic deleted the compound_condition_variables branch April 16, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A change that breaks existing models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control based on difference condition
2 participants