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

Make edge_id user configurable #1737

Merged
merged 5 commits into from
Aug 20, 2024
Merged

Make edge_id user configurable #1737

merged 5 commits into from
Aug 20, 2024

Conversation

evetion
Copy link
Member

@evetion evetion commented Aug 19, 2024

Fixes #1718

Sets node_id and edge_id as primary key fields in the geopackage. Also sets fid indexes for all non-node/edge tables. This now sorts the Node table by node_id only (used to be node_type first).

I triggered some bugs by the amount of code I touched, so I changed some more:

  • Split contextvars into reading and writing (arrow file was trying to be read on writing a model).
  • Simplified sorting
  • Moved all the DataFrame(dict(**kwargs)) boilerplate into a single validator on TableModel.

@evetion evetion requested a review from visr August 19, 2024 20:22
@visr
Copy link
Member

visr commented Aug 20, 2024

Am I correct that this is only breaking in the sense that the new version expects edge_id in Edge, whereas old models don't have that. But reading and writing with Python will fix that? Can we read old models with Python or will it fail on the presence of a fid column?

@evetion
Copy link
Member Author

evetion commented Aug 20, 2024

Am I correct that this is only breaking in the sense that the new version expects edge_id in Edge, whereas old models don't have that. But reading and writing with Python will fix that? Can we read old models with Python or will it fail on the presence of a fid column?

Correct on both. You can read older models with Python, as pyogrio hardcodes the index to fid anyway, we rename it afterwards. 😄

@visr visr added the breaking A change that breaks existing models label Aug 20, 2024
python/ribasim/ribasim/utils.py Outdated Show resolved Hide resolved
@visr
Copy link
Member

visr commented Aug 20, 2024

Regarding this point from #1718:

The non-spatial tables (Basin / static etc) also get an fid, that falls outside our data model but was needed to make the tables editable in QGIS. This was done in #873. To avoid confusion, Marnix tried to hide these columns in #841 (comment) but ran into a QGIS bug that has since been fixed. So we should try again to hide these columns for non-spatial tables.

Should we create an issue to start hiding fid columns in QGIS again? Am I correct that fid is now carried over, an no longer reset each time, by Ribasim Python? But it still isn't really part of the data model.

A bit of a nit, but in QGIS node_id shows up as the first column, but edge_id as the last. Can we make edge_id the first as well?

image

This id column is hardcoded in the iMOD QGIS plugin to fid, but that doesn't affect anything other than a confusing name:

image

https://github.com/Deltares/imod-qgis/blob/afa0eb694d367999c328e0dffc45f0bafb8230d1/imodqgis/timeseries/timeseries_widget.py#L471

@evetion
Copy link
Member Author

evetion commented Aug 20, 2024

Should we create an issue to start hiding fid columns in QGIS again? Am I correct that fid is now carried over, an no longer reset each time, by Ribasim Python? But it still isn't really part of the data model.

I tried to make things consistent by having the indexes always there in Python with the same name as well, not just in geopackage. While the fid index resets during normal use (sorting when writing destroys the original fid), it should carry over to and from QGIS. We can make an issue, if you dislike seeing the fid, but I don't care that much either way.

A bit of a nit, but in QGIS node_id shows up as the first column, but edge_id as the last. Can we make edge_id the first as well?

Not sure why that happens, in the table schema it shows up as the first column.

@visr
Copy link
Member

visr commented Aug 20, 2024

Great, let's merge then. The fid in the tables don't bother me that much either. edge_id on the left would be nice, but perhaps not under our control then.

@evetion evetion merged commit 1f2e303 into main Aug 20, 2024
27 of 29 checks passed
@evetion evetion deleted the feat/edge-id branch August 20, 2024 13:16
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.

Use of fid column in Geopackage
2 participants