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

Write full Node table once #1312

Merged
merged 5 commits into from
Mar 28, 2024
Merged

Write full Node table once #1312

merged 5 commits into from
Mar 28, 2024

Conversation

visr
Copy link
Member

@visr visr commented Mar 25, 2024

No description provided.

Hofer-Julian pushed a commit that referenced this pull request Mar 26, 2024
The idea was that the Node table `fid` would not be used anymore, but
#1306 showed that is unfortunately not yet the case. So this changes the
`fid` after it is initially written to be equal to the `node_id`. Any
users should still not rely on the `fid`, because this is only
temporary.

This is like #1311 but now also catching non unique node IDs on
`model.write`.
It also provides a temporary fix to #1306. It's not great, but I feel
that especially #1306 is bad enough to warrant a quick fix. This
would've been a bit cleaner to implement using #1312 to avoid having to
read and re-write the Node table, but that PR still has issues we don't
understand, so best not to wait on that.


![image](https://github.com/Deltares/Ribasim/assets/4471859/3efc2f8c-6be5-459d-929a-7ce8568fca3d)
@Hofer-Julian Hofer-Julian marked this pull request as ready for review March 28, 2024 11:58
Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

I approve my own changes

@visr visr merged commit af6b1ad into main Mar 28, 2024
24 checks passed
@visr visr deleted the nonodetable branch March 28, 2024 12:22
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.

2 participants