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.

Sign up for GitHub

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

Jump to bottom

Stop requiring globally unique node ids #1318

Closed
9 tasks done
Hofer-Julian opened this issue Mar 26, 2024 · 2 comments · Fixed by #1513
Closed
9 tasks done

Stop requiring globally unique node ids #1318

Hofer-Julian opened this issue Mar 26, 2024 · 2 comments · Fixed by #1513
Labels
core Issues related to the computational core in Julia python Relates to one of the Ribasim python packages QGIS Ribasim QGIS plugin tech-debt Improvements related to technical debt

Comments

@Hofer-Julian
Copy link
Contributor

Hofer-Julian commented Mar 26, 2024 •
edited by visr
Loading

We recently moved away from requiring globally unique node ids.
However, parts of our code still require it implicitly.
Because of that, we added checks to require it after all.

Tasks:
@Hofer-Julian Hofer-Julian added QGIS Ribasim QGIS plugin core Issues related to the computational core in Julia tech-debt Improvements related to technical debt labels Mar 26, 2024
@github-project-automation github-project-automation bot moved this to To do in Ribasim Mar 26, 2024
@Hofer-Julian Hofer-Julian added the python Relates to one of the Ribasim python packages label Mar 26, 2024
@visr visr changed the title Technical dept: stop requiring globally unique node ids Stop requiring globally unique node ids Mar 26, 2024
@visr visr mentioned this issue Mar 26, 2024
Merged
@Hofer-Julian
Copy link
Contributor Author

Sounds like this can be (nearly?) closed

@visr
Copy link
Member

visr commented Apr 17, 2024

I just unchecked some boxes, which were ticked since the related PRs were merged. But the task revert #x is not done when #x is merged.

@visr visr moved this from To do to 🏗 In progress in Ribasim May 30, 2024
@visr visr mentioned this issue May 30, 2024
Merged
@evetion evetion moved this from 🏗 In progress to 👀 In review in Ribasim Jun 5, 2024
evetion added a commit that referenced this issue Jun 5, 2024
@visr @evetion
Don't require unique node IDs (#1513)
89b8574 
Fixes #1318
Fixes #1256

@evetion I didn't touch the Delwaq `node_lookup`. If only Basins are in
the graph, then you can still use an ID as a unique index.

The #1256 fix in 72dcf24 is not tested,
since Ribasim-Python stops us from creating such an invalid model. I
manually confirmed it works by duplicating rows in QGIS.

The `main` methods were needed to fix `pixi run ribasim-core`.

---------

Co-authored-by: Maarten Pronk <[email protected]>
Co-authored-by: Maarten Pronk <[email protected]>
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Ribasim Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
core Issues related to the computational core in Julia python Relates to one of the Ribasim python packages QGIS Ribasim QGIS plugin tech-debt Improvements related to technical debt
Projects
Ribasim
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't require unique node IDs Deltares/Ribasim
2 participants
@visr @Hofer-Julian