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 node ID unique to the node type #1113

Merged
merged 12 commits into from
Feb 12, 2024
Merged

Make node ID unique to the node type #1113

merged 12 commits into from
Feb 12, 2024

Conversation

visr
Copy link
Member

@visr visr commented Feb 12, 2024

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 a node_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 the Edge table now gets from_node_type next to from_node_id, the PidControl / static gets listen_node_type next to listen_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 to NodeID and fixes the resulting errors.

struct NodeID
    type::NodeType.T
    value::Int
end

It does not yet test if models with the same node IDs (Pump #1 and Basin #1) work, but this is hard to do right now with Ribasim Python, so best left for a later moment.

Comment on lines -61 to -76
@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

Copy link
Member Author

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.

Copy link
Member Author

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.

@Hofer-Julian
Copy link
Contributor

Julia Tests are still red :)

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.

A few small nitpicks, but apart from that it looks good

core/src/util.jl Outdated Show resolved Hide resolved
python/ribasim/ribasim/model.py Show resolved Hide resolved
@visr visr merged commit a39cc24 into main Feb 12, 2024
21 checks passed
@visr visr deleted the id branch February 12, 2024 20:57
Hofer-Julian added a commit that referenced this pull request Feb 19, 2024
visr pushed a commit that referenced this pull request Feb 19, 2024
Hofer-Julian added a commit that referenced this pull request Feb 22, 2024
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