Skip to content

Make node_id globally unique (again)#1717

Merged
Jingru923 merged 9 commits intomainfrom
feat/unique-id
Aug 13, 2024
Merged

Make node_id globally unique (again)#1717
Jingru923 merged 9 commits intomainfrom
feat/unique-id

Conversation

@evetion
Copy link
Member

@evetion evetion commented Aug 12, 2024

Fixes #1690 as a 🎁 for @deltamarnix when he is back from holiday.

Triggered by #1648, which seems very hard to do in QGIS as it doesn't support compound indexes and we dislike a separate global unique id. In a discussion with @visr he mentioned most people don't use the non-global unique id feature (yet).

Reverts #1513. Partially reverts #1690 as the NodeID changes are not reverted, as it is quite useful to know the node_type in several parts of the code (instead of only a node_id). Therefore, I've added a new NodeID constructor and joined the Edge table with the Node table.

I had to change the node_ids of several new test models created by @SouthEndMusic, bit of a pain, but the tests seem to pass still.

Furthermore, I changed the node.add functionality to return the NodeData, which should make the edge.add more straightforward (and pave the way for automatic id numbering in the future). So this:

model.terminal.add(Node(terminal_id, Point(500, 200)))
model.tabulated_rating_curve.add(
    Node(6, Point(450, 200)),
    [tabulated_rating_curve.Static(level=[0.0, 1.0], flow_rate=[0.0, 10 / 86400])],
)

model.edge.add(
    model.basin[6],
    model.tabulated_rating_curve[6],
)
model.edge.add(
    model.tabulated_rating_curve[6],
    model.terminal[terminal_id],
)

becomes this:

term = model.terminal.add(Node(terminal_id, Point(500, 200)))
trc0 = model.tabulated_rating_curve.add(
    Node(0, Point(450, 200)),
    [tabulated_rating_curve.Static(level=[0.0, 1.0], flow_rate=[0.0, 10 / 86400])],
)

model.edge.add(
    basin6,
    trc0,
)
model.edge.add(trc0, term)

I've made this change in a single Python test model, and in the first model of the examples notebook. We could gradually change this further over time.

@evetion evetion requested a review from visr August 12, 2024 18:34
@evetion
Copy link
Member Author

evetion commented Aug 13, 2024

Given my frustration with renumbering some odd models, I've added automagic numbering of NodeIDs to this PR, which also includes quicker validation:

model.terminal.add(Node(1, Point(500, 200)))
model.terminal.add(Node(1, Point(500, 200)))  # will now error (before only when writing)

And do the following to get automagic numbering (you have to specify the geometry kwarg for constructor reasons, best to move to fully named arguments in the future):

term = model.terminal.add(Node(geometry=Point(500, 200)))
assert term.node_id == 1

@visr visr added the breaking A change that breaks existing models label Aug 13, 2024
Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

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

Great work! Left some comments.

@visr visr mentioned this pull request Aug 13, 2024
evetion and others added 3 commits August 13, 2024 15:05
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.

Go back to globally unique node IDs?

3 participants