From e0e3612c6ed746b4fcb687adb62bfd16ec851108 Mon Sep 17 00:00:00 2001 From: Herman Sletmoen Date: Tue, 16 Jul 2024 20:05:48 +0200 Subject: [PATCH 1/6] Delegate getdefault(var) to Symbolics, which already errors if var has no default --- src/utils.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils.jl b/src/utils.jl index dc239e34c1..4ee221e14f 100644 --- a/src/utils.jl +++ b/src/utils.jl @@ -217,7 +217,7 @@ function iv_from_nested_derivative(x, op = Differential) end hasdefault(v) = hasmetadata(v, Symbolics.VariableDefaultValue) -getdefault(v) = value(getmetadata(v, Symbolics.VariableDefaultValue)) +getdefault(v) = value(Symbolics.getdefaultval(v)) function getdefaulttype(v) def = value(getmetadata(unwrap(v), Symbolics.VariableDefaultValue, nothing)) def === nothing ? Float64 : typeof(def) From 170d7892c4463ab2d43d842ce96682fa08a6eb8f Mon Sep 17 00:00:00 2001 From: Herman Sletmoen Date: Tue, 16 Jul 2024 20:06:18 +0200 Subject: [PATCH 2/6] Add tests --- test/variable_parsing.jl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/variable_parsing.jl b/test/variable_parsing.jl index 14c6ca543e..e256611dc4 100644 --- a/test/variable_parsing.jl +++ b/test/variable_parsing.jl @@ -104,6 +104,7 @@ end y = 2, [connect = Flow] end +@test_throws ErrorException ModelingToolkit.getdefault(x) @test !hasmetadata(x, VariableDefaultValue) @test getmetadata(x, VariableConnectType) == Flow @test getmetadata(x, VariableUnit) == u @@ -111,6 +112,7 @@ end @test getmetadata(y, VariableConnectType) == Flow a = rename(value(x), :a) +@test_throws ErrorException ModelingToolkit.getdefault(x) @test !hasmetadata(x, VariableDefaultValue) @test getmetadata(x, VariableConnectType) == Flow @test getmetadata(x, VariableUnit) == u From a3ad348eec1c2b8dbf12c5fb5d02493f0747b4c5 Mon Sep 17 00:00:00 2001 From: Herman Sletmoen Date: Tue, 16 Jul 2024 20:08:12 +0200 Subject: [PATCH 3/6] Correct test to test what it was intended to test --- test/variable_parsing.jl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/variable_parsing.jl b/test/variable_parsing.jl index e256611dc4..1a5b0ed221 100644 --- a/test/variable_parsing.jl +++ b/test/variable_parsing.jl @@ -112,10 +112,10 @@ end @test getmetadata(y, VariableConnectType) == Flow a = rename(value(x), :a) -@test_throws ErrorException ModelingToolkit.getdefault(x) -@test !hasmetadata(x, VariableDefaultValue) -@test getmetadata(x, VariableConnectType) == Flow -@test getmetadata(x, VariableUnit) == u +@test_throws ErrorException ModelingToolkit.getdefault(a) +@test !hasmetadata(a, VariableDefaultValue) +@test getmetadata(a, VariableConnectType) == Flow +@test getmetadata(a, VariableUnit) == u @variables t x(t)=1 [connect = Flow, unit = u] From 47cdd3b3af7f1b0b6cb0b1a2ab659c8cdb489f8c Mon Sep 17 00:00:00 2001 From: Herman Sletmoen Date: Tue, 16 Jul 2024 21:15:45 +0200 Subject: [PATCH 4/6] Check that equations contain variables --- src/utils.jl | 19 ++++++++++++++++++- test/odesystem.jl | 9 ++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/utils.jl b/src/utils.jl index 4ee221e14f..47012b012c 100644 --- a/src/utils.jl +++ b/src/utils.jl @@ -171,7 +171,7 @@ end """ check_equations(eqs, iv) -Assert that equations are well-formed when building ODE, i.e., only containing a single independent variable. +Assert that ODE equations are well-formed. """ function check_equations(eqs, iv) ivs = collect_ivs(eqs) @@ -183,6 +183,17 @@ function check_equations(eqs, iv) isequal(single_iv, iv) || throw(ArgumentError("Differential w.r.t. variable ($single_iv) other than the independent variable ($iv) are not allowed.")) end + + for eq in eqs + vars, pars = collect_vars(eq, iv) + if isempty(vars) + if isempty(pars) + throw(ArgumentError("Equation $eq contains no variables or parameters.")) + else + throw(ArgumentError("Equation $eq contains only parameters, but relationships between parameters should be specified with defaults or parameter_dependencies.")) + end + end + end end """ Get all the independent variables with respect to which differentials are taken. @@ -439,6 +450,12 @@ function find_derivatives!(vars, expr, f) return vars end +function collect_vars(args...; kwargs...) + unknowns, parameters = [], [] + collect_vars!(unknowns, parameters, args...; kwargs...) + return unknowns, parameters +end + function collect_vars!(unknowns, parameters, expr, iv; op = Differential) if issym(expr) collect_var!(unknowns, parameters, expr, iv) diff --git a/test/odesystem.jl b/test/odesystem.jl index ab28f5cb47..04331008a1 100644 --- a/test/odesystem.jl +++ b/test/odesystem.jl @@ -420,7 +420,14 @@ der = Differential(w) eqs = [ der(u1) ~ t ] -@test_throws ArgumentError ModelingToolkit.ODESystem(eqs, t, vars, pars, name = :foo) +@test_throws ArgumentError ODESystem(eqs, t, vars, pars, name = :foo) + + # equations without variables are forbidden + # https://github.com/SciML/ModelingToolkit.jl/issues/2727 +@parameters p q +@test_throws ArgumentError ODESystem([p ~ q], t; name = :foo) +@test_throws ArgumentError ODESystem([p ~ 1], t; name = :foo) +@test_throws ArgumentError ODESystem([1 ~ 2], t; name = :foo) @variables x(t) @parameters M b k From 836bc2e50498a872f2c45d0db4ee8c39d1f8b7a8 Mon Sep 17 00:00:00 2001 From: Herman Sletmoen Date: Tue, 16 Jul 2024 21:27:32 +0200 Subject: [PATCH 5/6] Revert "Check that equations contain variables" (commited to wrong branch and pushed it...) This reverts commit 47cdd3b3af7f1b0b6cb0b1a2ab659c8cdb489f8c. --- src/utils.jl | 19 +------------------ test/odesystem.jl | 9 +-------- 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/src/utils.jl b/src/utils.jl index 47012b012c..4ee221e14f 100644 --- a/src/utils.jl +++ b/src/utils.jl @@ -171,7 +171,7 @@ end """ check_equations(eqs, iv) -Assert that ODE equations are well-formed. +Assert that equations are well-formed when building ODE, i.e., only containing a single independent variable. """ function check_equations(eqs, iv) ivs = collect_ivs(eqs) @@ -183,17 +183,6 @@ function check_equations(eqs, iv) isequal(single_iv, iv) || throw(ArgumentError("Differential w.r.t. variable ($single_iv) other than the independent variable ($iv) are not allowed.")) end - - for eq in eqs - vars, pars = collect_vars(eq, iv) - if isempty(vars) - if isempty(pars) - throw(ArgumentError("Equation $eq contains no variables or parameters.")) - else - throw(ArgumentError("Equation $eq contains only parameters, but relationships between parameters should be specified with defaults or parameter_dependencies.")) - end - end - end end """ Get all the independent variables with respect to which differentials are taken. @@ -450,12 +439,6 @@ function find_derivatives!(vars, expr, f) return vars end -function collect_vars(args...; kwargs...) - unknowns, parameters = [], [] - collect_vars!(unknowns, parameters, args...; kwargs...) - return unknowns, parameters -end - function collect_vars!(unknowns, parameters, expr, iv; op = Differential) if issym(expr) collect_var!(unknowns, parameters, expr, iv) diff --git a/test/odesystem.jl b/test/odesystem.jl index 04331008a1..ab28f5cb47 100644 --- a/test/odesystem.jl +++ b/test/odesystem.jl @@ -420,14 +420,7 @@ der = Differential(w) eqs = [ der(u1) ~ t ] -@test_throws ArgumentError ODESystem(eqs, t, vars, pars, name = :foo) - - # equations without variables are forbidden - # https://github.com/SciML/ModelingToolkit.jl/issues/2727 -@parameters p q -@test_throws ArgumentError ODESystem([p ~ q], t; name = :foo) -@test_throws ArgumentError ODESystem([p ~ 1], t; name = :foo) -@test_throws ArgumentError ODESystem([1 ~ 2], t; name = :foo) +@test_throws ArgumentError ModelingToolkit.ODESystem(eqs, t, vars, pars, name = :foo) @variables x(t) @parameters M b k From a9e0c6b3e2f6bdf40d53d291e462a9f710551a27 Mon Sep 17 00:00:00 2001 From: Herman Sletmoen Date: Tue, 16 Jul 2024 23:36:52 +0200 Subject: [PATCH 6/6] Update old test --- test/model_parsing.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/model_parsing.jl b/test/model_parsing.jl index d8665190c8..7ca5618786 100644 --- a/test/model_parsing.jl +++ b/test/model_parsing.jl @@ -263,7 +263,7 @@ end @test getdefault(model.cval) == 1 @test isequal(getdefault(model.c), model.cval + model.jval) @test getdefault(model.d) == 2 - @test_throws KeyError getdefault(model.e) + @test_throws ErrorException getdefault(model.e) @test getdefault(model.f) == 3 @test getdefault(model.i) == 4 @test all(getdefault.(scalarize(model.b2)) .== [1, 3])