-
Notifications
You must be signed in to change notification settings - Fork 5
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 node ID unique to the node type #1113
Conversation
@version NodeV1 begin | ||
fid::Int | ||
name::String = isnothing(s) ? "" : String(s) | ||
type::String = in(Symbol(type), nodetypes) ? type : error("Unknown node type $type") | ||
allocation_network_id::Union{Missing, Int} | ||
end | ||
|
||
@version EdgeV1 begin | ||
fid::Int | ||
name::String = isnothing(s) ? "" : String(s) | ||
from_node_id::Int | ||
to_node_id::Int | ||
edge_type::String | ||
allocation_network_id::Union{Missing, Int} | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These schemas were not used in the core, and their translations in Python were also not used, they are handwritten elsewhere. So best to just remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed when Ribasim-Python is ready for it.
Julia Tests are still red :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small nitpicks, but apart from that it looks good
Needed for #1110.
Before node IDs were globally unique. If you had the ID, you could look up the type in the Node table. When designing #1110 we came to the conclusion that this was not the right choice. It requires you to be aware of the IDs that are used throughout the model, whereas with this you only need to make sure that
Pump #5
doesn't already exist.Most of the updates in this PR were to correct the tests. Some of the code and error messages became easier to read. If we talk about a node then we always know what the type is, there is no need to look it up first.
Most tables stay the same, e.g. if you have a
Terminal / static
table with anode_id
Int column, you know that this refers to a Terminal NodeID. Only when connecting to other nodes do we need to specify the type next to the ID. So theEdge
table now getsfrom_node_type
next tofrom_node_id
, thePidControl / static
getslisten_node_type
next tolisten_node_id
, etc. These extra columns are currently automatically filled in by Ribasim-Python on model save, hence they don't require changing the test models.In terms of implementation, this basically adds the
type
field toNodeID
and fixes the resulting errors.It does not yet test if models with the same node IDs (
Pump #1
andBasin #1
) work, but this is hard to do right now with Ribasim Python, so best left for a later moment.