Ribasim icon indicating copy to clipboard operation
Ribasim copied to clipboard

Refactor control (input)

Open SouthEndMusic opened this issue 2 years ago • 7 comments

There has been some discussion about refactoring the control implementation. Here are some suggestions, but there should be more of a consensus on this before it is implemented.

This one I think is uncontroversial:

  • [ ] Rename control_state to control_state_id and use integers everywhere in stead of strings.

But these ones need refinement:

  • [ ] Use a binary vector (or integer from binary) representation of truth states in the Julia core in stead of strings with "FTTFFTTFF"
  • [ ] Change the logic schema so that not all truth states have to be represented explicitly.

Regarding the representation of the truth states in the input tables: a string representation is more user friendly, a binary or integer representation is more memory and processing friendly.

Or the logic table could be structured differently, more in terms of logical operators (AND/OR/...)

SouthEndMusic avatar Jun 12 '23 11:06 SouthEndMusic

I would recommend 'control_state_id' instead of 'control_id'

gijsber avatar Jun 12 '23 11:06 gijsber

After mostly arguing with myself, here are some of my thoughts:

  • The current approach has essentially no syntax and is explicit.
  • The current approach is complete: if you have very complex logic, you just have to fully enumerate your options.

In general, there shouldn't be much problems (finger crossed) with the current approach as long as the number of conditions remains small (let's say <=3). It seems likely that this is the case, nearly all of the time. In that case, there aren't big problems:

  • 2^3 is 8 rows in a table, which is manageable.
  • Similarly, remembering three conditions is very doable (so TTT is not that hard to debug).
  • If you do run into (hopefully very rare) a big case, you can write a simple Python program to generate all permutations.

To make it slightly easier to setup, we could add a "default" / "rest" / "remainder" truth state. I think this should be opt-in explicitly.

Huite avatar Jun 12 '23 13:06 Huite

  • [ ] Split the static and control tables in the node type data tables

visr avatar Jun 16 '23 14:06 visr

Split the static and control tables in the node type data table

Might be good to discuss this and weigh the pros and cons? Given our tech-debt for not automatically generating other tables (and names/casing of them), this would result in doubling tables, especially if we implement control for more node and subtypes. I also don't think it's an improvement from a user point of view.

evetion avatar Jun 16 '23 17:06 evetion

I think it's a bit hard to explain that static and control states are combined in one table, but we still have time tables separately. Having to filter rows to look for a specific type of data isn't that user friendly to me either. You don't necessarily need both static and control data for each node type.

I'd be up for thinking about how we can manage our tables and code to make it easy to maintain and configure. But the decision to combine static and control seems inconsistent with the general design. That is why it was separate in the proposal.

visr avatar Jun 16 '23 19:06 visr

We need a decision whether this indeed needed.

SnippenE avatar Jun 20 '23 14:06 SnippenE

One major pain point is the ordering of the characters in the truth state, FFT. The order should be the same as the sorted DiscreteControl / condition table. But this is not clear to users, who will put them in the input order of the condition table rather than the sorted version. See #1117 for an example.

visr avatar Feb 13 '24 11:02 visr