From 764f23d8b014714e3c49a9143de9f5f98a3a5ac8 Mon Sep 17 00:00:00 2001 From: Jingru Feng Date: Thu, 1 Feb 2024 14:02:28 +0100 Subject: [PATCH 1/4] made an invalid model that misses edge between node added validation for positive allocation id and for non-negative user demand Fix the bug in program, the last second validation function is working Removed commented code Validation of the second last input is now done inside inner constructor. Code of validation is now a function in validation.jl made an draft test case, which is unfinished fixed the bug in validation.jl that 'ScalarInterpolation' can't be recognized add function to check the connection of the subnetworks the code for checking subnetwork connection is now bug-free made unit test for negative demand Created "create_test.jl" and wrote part of the incomplete subnetwork test created unit test for non-positive allocation id validating function and test allocation id of 0 and -1 removed unnecessary Ribasim Python model finished the unit test for the subnetwork connection validation function Resolved Bart's comment on PR --- Project.toml | 3 ++ core/src/Ribasim.jl | 4 +- core/src/create.jl | 8 ++- core/src/solve.jl | 26 +++++++++ core/src/utils.jl | 4 ++ core/src/validation.jl | 47 ++++++++++++++++ core/test/create_test.jl | 100 +++++++++++++++++++++++++++++++++++ core/test/validation_test.jl | 24 +++++++++ 8 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 core/test/create_test.jl diff --git a/Project.toml b/Project.toml index 36cc6a120..28229c299 100644 --- a/Project.toml +++ b/Project.toml @@ -3,6 +3,9 @@ authors = ["Deltares and contributors "] description = "Meta-project used to share the Manifest of Ribasim and its dependents" [deps] +DataInterpolations = "82cc6244-b520-54b8-b5a6-8a565e85f1d0" +Graphs = "86223c79-3864-5bf0-83f7-82e725a168b6" +MetaGraphsNext = "fa8bd995-216d-47f1-8a91-f3b68fbeb377" ReTestItems = "817f1d60-ba6b-4fd5-9520-3cf149f6a823" Revise = "295af30f-e4ad-537b-8983-00126c2a3abe" Ribasim = "aac5e3d9-0b8f-4d4f-8241-b1a7a9632635" diff --git a/core/src/Ribasim.jl b/core/src/Ribasim.jl index 79947b8a6..c18d04188 100644 --- a/core/src/Ribasim.jl +++ b/core/src/Ribasim.jl @@ -44,7 +44,9 @@ using Graphs: inneighbors, nv, outneighbors, - rem_edge! + rem_edge!, + induced_subgraph, + is_connected using Legolas: Legolas, @schema, @version, validate, SchemaVersion, declared using Logging: with_logger, LogLevel, AbstractLogger diff --git a/core/src/create.jl b/core/src/create.jl index 46a5f9ada..c4fb44f31 100644 --- a/core/src/create.jl +++ b/core/src/create.jl @@ -199,6 +199,12 @@ const nonconservative_nodetypes = function generate_allocation_models!(p::Parameters, config::Config)::Nothing (; graph, allocation_models) = p + errors = non_positive_id(graph) + + if errors + error("Allocation network initialization failed.") + end + for allocation_network_id in keys(graph[].node_ids) push!( allocation_models, @@ -691,7 +697,7 @@ function User(db::DB, config::Config)::User first_row = time[first_row_idx] is_active = true else - @error "User node #$node_id data not in any table." + @error "User node $node_id data not in any table." errors = true end diff --git a/core/src/solve.jl b/core/src/solve.jl index ab37fd191..56dae81a2 100644 --- a/core/src/solve.jl +++ b/core/src/solve.jl @@ -415,6 +415,32 @@ struct User <: AbstractParameterNode allocated::Vector{Float64}, abstracted::Vector{Float64}, } + + function User( + node_id, + active, + demand, + allocated, + return_factor, + min_level, + priorities, + record, + ) + if valid_demand(node_id, demand, priorities) + return new( + node_id, + active, + demand, + allocated, + return_factor, + min_level, + priorities, + record, + ) + else + error("Invalid demand") + end + end end "Subgrid linearly interpolates basin levels." diff --git a/core/src/utils.jl b/core/src/utils.jl index 5040f786c..d27cd00d3 100644 --- a/core/src/utils.jl +++ b/core/src/utils.jl @@ -88,6 +88,10 @@ function create_graph(db::DB, config::Config, chunk_sizes::Vector{Int})::MetaGra end end + if incomplete_subnetwork(graph, node_ids) + error("Incomplete connectivity in subnetwork") + end + flow = zeros(flow_counter) flow_vertical = zeros(flow_vertical_counter) if config.solver.autodiff diff --git a/core/src/validation.jl b/core/src/validation.jl index d49dc3506..04b914b10 100644 --- a/core/src/validation.jl +++ b/core/src/validation.jl @@ -660,3 +660,50 @@ function valid_subgrid( return !errors end + +function valid_demand( + node_id::Vector{NodeID}, + demand::Vector{ + Vector{LinearInterpolation{Vector{Float64}, Vector{Float64}, true, Float64}}, + }, + priorities::Vector{Int}, +)::Bool + errors = false + + for (col, id) in zip(demand, node_id) + for (demand_p_itp, p_itp) in zip(col, priorities) + if any(demand_p_itp.u .< 0.0) + @error "Demand of user node $id with priority $p_itp should be non-negative" + errors = true + end + end + end + return !errors +end + +function valid_connection(graph::MetaGraph)::Bool + return is_connected(graph) +end + +function incomplete_subnetwork(graph::MetaGraph, node_ids::Dict{Int, Set{NodeID}})::Bool + errors = false + for (allocation_network_id, subnetwork_node_ids) in node_ids + subnetwork, _ = induced_subgraph(graph, code_for.(Ref(graph), subnetwork_node_ids)) + if (!is_connected(subnetwork)) + @error "All nodes in subnetwork $allocation_network_id should be connected" + errors = true + end + end + return errors +end + +function non_positive_id(graph::MetaGraph)::Bool + errors = false + for allocation_network_id in keys(graph[].node_ids) + if (allocation_network_id <= 0) + @error "Allocation network id $allocation_network_id needs to be a positive integer." + errors = true + end + end + return errors +end diff --git a/core/test/create_test.jl b/core/test/create_test.jl new file mode 100644 index 000000000..35b0809db --- /dev/null +++ b/core/test/create_test.jl @@ -0,0 +1,100 @@ +@testitem "Non-positive allocation network ID" begin + using MetaGraphsNext + using Graphs + using Logging + using Ribasim + using Accessors: @set + + struct NodeMetadata + type::Symbol + allocation_network_id::Int + end + + graph = MetaGraph( + DiGraph(); + label_type = Ribasim.NodeID, + vertex_data_type = NodeMetadata, + edge_data_type = Symbol, + graph_data = Tuple, + ) + + graph[Ribasim.NodeID(1)] = NodeMetadata(Symbol(:delft), 1) + graph[Ribasim.NodeID(2)] = NodeMetadata(Symbol(:denhaag), -1) + + graph[1, 2] = :yes + + node_ids = Dict{Int, Set{Ribasim.NodeID}}() + node_ids[0] = Set{Ribasim.NodeID}() + node_ids[-1] = Set{Ribasim.NodeID}() + push!(node_ids[0], Ribasim.NodeID(1)) + push!(node_ids[-1], Ribasim.NodeID(2)) + + graph_data = (; node_ids,) + graph = @set graph.graph_data = graph_data + + logger = TestLogger() + with_logger(logger) do + Ribasim.non_positive_id(graph) + end + + @test length(logger.logs) == 2 + @test logger.logs[1].level == Error + @test logger.logs[1].message == + "Allocation network id 0 needs to be a positive integer." + @test logger.logs[2].level == Error + @test logger.logs[2].message == + "Allocation network id -1 needs to be a positive integer." +end + +@testitem "Incomplete subnetwork" begin + using MetaGraphsNext + using Graphs + using Logging + using Ribasim + + struct NodeMetadata + type::Symbol + allocation_network_id::Int + end + + graph = MetaGraph( + DiGraph(); + label_type = Ribasim.NodeID, + vertex_data_type = NodeMetadata, + edge_data_type = Symbol, + graph_data = Tuple, + ) + + node_ids = Dict{Int, Set{Ribasim.NodeID}}() + node_ids[1] = Set{Ribasim.NodeID}() + push!(node_ids[1], Ribasim.NodeID(1)) + push!(node_ids[1], Ribasim.NodeID(2)) + push!(node_ids[1], Ribasim.NodeID(3)) + node_ids[2] = Set{Ribasim.NodeID}() + push!(node_ids[2], Ribasim.NodeID(4)) + push!(node_ids[2], Ribasim.NodeID(5)) + push!(node_ids[2], Ribasim.NodeID(5)) + #node_ids = Dict([(1, Set(NodeID(1))), (2, Set(NodeID(2)))]) + + graph[Ribasim.NodeID(1)] = NodeMetadata(Symbol(:delft), 1) + graph[Ribasim.NodeID(2)] = NodeMetadata(Symbol(:denhaag), 1) + graph[Ribasim.NodeID(3)] = NodeMetadata(Symbol(:rdam), 1) + graph[Ribasim.NodeID(4)] = NodeMetadata(Symbol(:adam), 2) + graph[Ribasim.NodeID(5)] = NodeMetadata(Symbol(:utrecht), 2) + graph[Ribasim.NodeID(6)] = NodeMetadata(Symbol(:leiden), 2) + + graph[Ribasim.NodeID(1), Ribasim.NodeID(2)] = :yes + graph[Ribasim.NodeID(1), Ribasim.NodeID(3)] = :yes + graph[4, 5] = :yes + + logger = TestLogger() + + with_logger(logger) do + errors = Ribasim.incomplete_subnetwork(graph, node_ids) + @test errors == true + end + + @test length(logger.logs) == 1 + @test logger.logs[1].level == Error + @test logger.logs[1].message == "All nodes in subnetwork 2 should be connected" +end diff --git a/core/test/validation_test.jl b/core/test/validation_test.jl index 1d6dd4ce4..df3d379fa 100644 --- a/core/test/validation_test.jl +++ b/core/test/validation_test.jl @@ -401,3 +401,27 @@ end @test logger.logs[2].message == "Basin / subgrid_level subgrid_id 1 has repeated element levels, this cannot be interpolated." end + +@testitem "negative demand" begin + using Logging + using DataInterpolations: LinearInterpolation + logger = TestLogger() + + with_logger(logger) do + @test_throws "Invalid demand" Ribasim.User( + [Ribasim.NodeID(1)], + [true], + [[LinearInterpolation([-5.0, -5.0], [-1.8, 1.8])]], + [0.0, -0.0], + [0.9], + [0.9], + [1], + [], + ) + end + + @test length(logger.logs) == 1 + @test logger.logs[1].level == Error + @test logger.logs[1].message == + "Demand of user node #1 with priority 1 should be non-negative" +end From df322b5f7cee782b7bb3d4701e18e612526a4241 Mon Sep 17 00:00:00 2001 From: Jingru Feng Date: Thu, 1 Feb 2024 15:40:42 +0100 Subject: [PATCH 2/4] resolved comment non_positive_allocation_network_id() --- core/src/create.jl | 2 +- core/src/validation.jl | 2 +- core/test/create_test.jl | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/create.jl b/core/src/create.jl index c4fb44f31..10d41ee5a 100644 --- a/core/src/create.jl +++ b/core/src/create.jl @@ -199,7 +199,7 @@ const nonconservative_nodetypes = function generate_allocation_models!(p::Parameters, config::Config)::Nothing (; graph, allocation_models) = p - errors = non_positive_id(graph) + errors = non_positive_allocation_network_id(graph) if errors error("Allocation network initialization failed.") diff --git a/core/src/validation.jl b/core/src/validation.jl index 2ab9e2b70..6102cbaeb 100644 --- a/core/src/validation.jl +++ b/core/src/validation.jl @@ -673,7 +673,7 @@ function incomplete_subnetwork(graph::MetaGraph, node_ids::Dict{Int, Set{NodeID} return errors end -function non_positive_id(graph::MetaGraph)::Bool +function non_positive_allocation_network_id(graph::MetaGraph)::Bool errors = false for allocation_network_id in keys(graph[].node_ids) if (allocation_network_id <= 0) diff --git a/core/test/create_test.jl b/core/test/create_test.jl index 35b0809db..c46b06cea 100644 --- a/core/test/create_test.jl +++ b/core/test/create_test.jl @@ -34,7 +34,7 @@ logger = TestLogger() with_logger(logger) do - Ribasim.non_positive_id(graph) + Ribasim.non_positive_allocation_network_id(graph) end @test length(logger.logs) == 2 From e99ab09c483fa5255aca12bfc662c732e9c6024b Mon Sep 17 00:00:00 2001 From: Jingru Feng Date: Thu, 1 Feb 2024 15:52:55 +0100 Subject: [PATCH 3/4] resolved Bart's second round of comments --- core/src/create.jl | 2 +- core/src/validation.jl | 6 +----- core/test/create_test.jl | 32 +++++++++++--------------------- 3 files changed, 13 insertions(+), 27 deletions(-) diff --git a/core/src/create.jl b/core/src/create.jl index 10d41ee5a..66e331e06 100644 --- a/core/src/create.jl +++ b/core/src/create.jl @@ -697,7 +697,7 @@ function User(db::DB, config::Config)::User first_row = time[first_row_idx] is_active = true else - @error "User node $node_id data not in any table." + @error "User node #$node_id data not in any table." errors = true end diff --git a/core/src/validation.jl b/core/src/validation.jl index 6102cbaeb..f74d03eb8 100644 --- a/core/src/validation.jl +++ b/core/src/validation.jl @@ -657,15 +657,11 @@ function valid_demand( return !errors end -function valid_connection(graph::MetaGraph)::Bool - return is_connected(graph) -end - function incomplete_subnetwork(graph::MetaGraph, node_ids::Dict{Int, Set{NodeID}})::Bool errors = false for (allocation_network_id, subnetwork_node_ids) in node_ids subnetwork, _ = induced_subgraph(graph, code_for.(Ref(graph), subnetwork_node_ids)) - if (!is_connected(subnetwork)) + if !is_connected(subnetwork) @error "All nodes in subnetwork $allocation_network_id should be connected" errors = true end diff --git a/core/test/create_test.jl b/core/test/create_test.jl index c46b06cea..b287d8ad2 100644 --- a/core/test/create_test.jl +++ b/core/test/create_test.jl @@ -5,21 +5,16 @@ using Ribasim using Accessors: @set - struct NodeMetadata - type::Symbol - allocation_network_id::Int - end - graph = MetaGraph( DiGraph(); label_type = Ribasim.NodeID, - vertex_data_type = NodeMetadata, + vertex_data_type = Ribasim.NodeMetadata, edge_data_type = Symbol, graph_data = Tuple, ) - graph[Ribasim.NodeID(1)] = NodeMetadata(Symbol(:delft), 1) - graph[Ribasim.NodeID(2)] = NodeMetadata(Symbol(:denhaag), -1) + graph[Ribasim.NodeID(1)] = Ribasim.NodeMetadata(Symbol(:delft), 1) + graph[Ribasim.NodeID(2)] = Ribasim.NodeMetadata(Symbol(:denhaag), -1) graph[1, 2] = :yes @@ -52,15 +47,10 @@ end using Logging using Ribasim - struct NodeMetadata - type::Symbol - allocation_network_id::Int - end - graph = MetaGraph( DiGraph(); label_type = Ribasim.NodeID, - vertex_data_type = NodeMetadata, + vertex_data_type = Ribasim.NodeMetadata, edge_data_type = Symbol, graph_data = Tuple, ) @@ -73,15 +63,15 @@ end node_ids[2] = Set{Ribasim.NodeID}() push!(node_ids[2], Ribasim.NodeID(4)) push!(node_ids[2], Ribasim.NodeID(5)) - push!(node_ids[2], Ribasim.NodeID(5)) + push!(node_ids[2], Ribasim.NodeID(6)) #node_ids = Dict([(1, Set(NodeID(1))), (2, Set(NodeID(2)))]) - graph[Ribasim.NodeID(1)] = NodeMetadata(Symbol(:delft), 1) - graph[Ribasim.NodeID(2)] = NodeMetadata(Symbol(:denhaag), 1) - graph[Ribasim.NodeID(3)] = NodeMetadata(Symbol(:rdam), 1) - graph[Ribasim.NodeID(4)] = NodeMetadata(Symbol(:adam), 2) - graph[Ribasim.NodeID(5)] = NodeMetadata(Symbol(:utrecht), 2) - graph[Ribasim.NodeID(6)] = NodeMetadata(Symbol(:leiden), 2) + graph[Ribasim.NodeID(1)] = Ribasim.NodeMetadata(Symbol(:delft), 1) + graph[Ribasim.NodeID(2)] = Ribasim.NodeMetadata(Symbol(:denhaag), 1) + graph[Ribasim.NodeID(3)] = Ribasim.NodeMetadata(Symbol(:rdam), 1) + graph[Ribasim.NodeID(4)] = Ribasim.NodeMetadata(Symbol(:adam), 2) + graph[Ribasim.NodeID(5)] = Ribasim.NodeMetadata(Symbol(:utrecht), 2) + graph[Ribasim.NodeID(6)] = Ribasim.NodeMetadata(Symbol(:leiden), 2) graph[Ribasim.NodeID(1), Ribasim.NodeID(2)] = :yes graph[Ribasim.NodeID(1), Ribasim.NodeID(3)] = :yes From 9f7a396ec8d25e0b2695fd0ad5bf1d7df5ceb838 Mon Sep 17 00:00:00 2001 From: Jingru Feng Date: Thu, 1 Feb 2024 16:14:30 +0100 Subject: [PATCH 4/4] remove unnecessary comment --- core/test/create_test.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/core/test/create_test.jl b/core/test/create_test.jl index b287d8ad2..946d6664f 100644 --- a/core/test/create_test.jl +++ b/core/test/create_test.jl @@ -64,7 +64,6 @@ end push!(node_ids[2], Ribasim.NodeID(4)) push!(node_ids[2], Ribasim.NodeID(5)) push!(node_ids[2], Ribasim.NodeID(6)) - #node_ids = Dict([(1, Set(NodeID(1))), (2, Set(NodeID(2)))]) graph[Ribasim.NodeID(1)] = Ribasim.NodeMetadata(Symbol(:delft), 1) graph[Ribasim.NodeID(2)] = Ribasim.NodeMetadata(Symbol(:denhaag), 1)