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

Remove Terminal / static as it is not needed #1209

Closed
visr opened this issue Mar 4, 2024 · 3 comments · Fixed by #1624
Closed

Remove Terminal / static as it is not needed #1209

visr opened this issue Mar 4, 2024 · 3 comments · Fixed by #1624

Comments

@visr
Copy link
Member

visr commented Mar 4, 2024

When updating models in #1110 I noticed that I can run a model with a Terminal node with or without the 'Terminal / static' table. Then I realized, especially with the new API, there is no real information in that table that isn't already in the Node table. We should look into if that is true, and if it is, perhaps we should just remove Terminal / static.

https://deltares.github.io/Ribasim/core/usage.html#sec-terminal

@SouthEndMusic
Copy link
Collaborator

SouthEndMusic commented Mar 12, 2024

Indeed in terms of information the Terminal / static table is not needed, since the only information associated with terminal nodes is their node ID which can also be found in the Node table.

Regarding the fact that you could run your model without this table: a quick search trough the core code showed that currently the terminal node data is only used in the corresponding formulate_flow! for interpreting the flow to a terminal node as a boundary flow. There being no data in the terminal object doesn't throw an error.

In conclusion: I think indeed the Terminal / static table can be removed, and for consistency in the core I think the Terminal constructor should read from the Node table (instead of not reading any data about terminal nodes at all).

@SouthEndMusic SouthEndMusic changed the title Is 'Terminal / static' needed or not? Is Terminal / static needed or not? Mar 12, 2024
@evetion
Copy link
Member

evetion commented Mar 13, 2024

I would advise against removing the table, as it would break the current generic code patterns, for just a small gain in lines removed. I would argue it increases complexity.

@visr
Copy link
Member Author

visr commented Mar 13, 2024

The table already doesn't appear in any of our test models. In the add API it becomes obvious.

model.terminal.add(Node(14, Point(3.0, -2.0)))
model.terminal.add(Node(14, Point(3.0, -2.0)), [terminal.Static()])

@gijsber gijsber added the v1.0 label Jul 15, 2024
@gijsber gijsber changed the title Is Terminal / static needed or not? Remove Terminal / static as it is not needed ? Jul 15, 2024
@gijsber gijsber changed the title Remove Terminal / static as it is not needed ? Remove Terminal / static as it is not needed Jul 15, 2024
visr added a commit that referenced this issue Jul 15, 2024
This also removes the Terminal type from the core, as it is not needed for the same reason. That meant I had to adapt a bit of code that relied on `getfield(p, :terminal)` working.

Fixes #1209
@visr visr moved this from To do to 👀 In review in Ribasim Jul 16, 2024
Jingru923 pushed a commit that referenced this issue Jul 16, 2024
This does not remove the Terminal type from the core, as it causes more
issues than it's worth, as suggested in #1209.

Fixes #1209
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Ribasim Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants