From 24489b00ddc32abcc547ecd209e12339848f97f4 Mon Sep 17 00:00:00 2001 From: Datseris Date: Wed, 30 Apr 2025 12:32:51 +0100 Subject: [PATCH 01/10] Check for wrong `Equation` type in RHS. --- CHANGELOG.md | 4 ++++ Project.toml | 2 +- src/make.jl | 18 ++++++++++++++++++ test/runtests.jl | 14 ++++++++++++++ 4 files changed, 37 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52a9bac..40db378 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ ProcessBasedModelling.jl follows semver 2.0. Changelog is kept with respect to v1 release. +## 1.6 + +- Added an additional step when constructing the raw equation vector to be passed into an MTK model. In this step it is also checked that the RHS for all equations is an `Expression`. Sometimes it is easy to get confused and mess up and make it be an `Equation` (i.e., assigning the LHS-variable twice). This now will give an informative error. + ## 1.5 - Add docstring to `processes_to_mtkeqs` and list it in the documentation. diff --git a/Project.toml b/Project.toml index 8eaed58..1a67bed 100644 --- a/Project.toml +++ b/Project.toml @@ -1,7 +1,7 @@ name = "ProcessBasedModelling" uuid = "ca969041-2cf3-4b10-bc21-86f4417093eb" authors = ["Datseris "] -version = "1.5.0" +version = "1.6.0" [deps] ModelingToolkit = "961ee093-0014-501f-94e3-6117800e7a78" diff --git a/src/make.jl b/src/make.jl index 684fb85..caf8e04 100644 --- a/src/make.jl +++ b/src/make.jl @@ -53,6 +53,10 @@ These registered processes are used when `default` is a `Module`. `t` is also exported by ProcessBasedModelling.jl for convenience. - `warn_default::Bool = true`: if `true`, throw a warning when a variable does not have an assigned process but it has a default value so that it becomes a parameter instead. +- `check_rhs::Bool = true`: if `true`, check that the RHS of all processes is NOT an + `Equation`. Throw an informative error if there is one. This + helps scenarios where the RHS is wrongly an `Equation` assigning the LHS itself + (has happened to me many times!). """ function processes_to_mtkmodel(args...; type = ODESystem, name = nameof(type), independent = t, kw..., @@ -79,8 +83,10 @@ processes_to_mtkeqs(procs, default_dict(v); kw...) # because this simplifies a bit the code function processes_to_mtkeqs(_processes::Vector, default::Dict{Num, Any}; type = ODESystem, name = nameof(type), independent = t, warn_default::Bool = true, + check_rhs = true, ) processes = expand_multi_processes(_processes) + check_rhs_validity(processes) # Setup: obtain lhs-variables so we can track new variables that are not # in this vector. The vector has to be of type `Num` lhs_vars = Num[lhs_variable(p) for p in processes] @@ -154,6 +160,18 @@ function expand_multi_processes(procs::Vector) return expanded end +function check_rhs_validity(processes) + for p in processes + if rhs(p) <: Equation + lvar = lhs_variable(p) + throw(ArgumentError("Process assigned to variable $(lvar) is ill defined. "* + "The RHS is an `<: Equation` type but it shouldn't be." + )) + end + end + return +end + function default_dict(processes) default = Dict{Num, Any}() for proc in processes diff --git a/test/runtests.jl b/test/runtests.jl index bfef75e..ef66916 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -215,6 +215,20 @@ end @test sort(ModelingToolkit.getname.(unknowns(sys2))) == [:w, :x, :y, :z] end +@testset "equation in RHS" begin + @variables z(t) = 0.0 + @variables x(t) = 0.0 + @variables y(t) = 0.0 + procs = [ + ExpRelaxation(z, x^2, 1.0), # introduces x and y variables + y ~ z-x, # is an equation, not a process! + z ~ (z ~ x^2), + ] + @test_throws ["an `<: Equation` type"] processes_to_mtkeqs(procs) +end + + + module TestDefault using ProcessBasedModelling @variables x(t) = 0.5 y(t) = 0.2 From c8bfc87334634b6fba3ee84c6fd97bc322887710 Mon Sep 17 00:00:00 2001 From: Datseris Date: Wed, 30 Apr 2025 12:34:14 +0100 Subject: [PATCH 02/10] oops use `isa` --- src/make.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/make.jl b/src/make.jl index caf8e04..b843f6b 100644 --- a/src/make.jl +++ b/src/make.jl @@ -54,7 +54,7 @@ These registered processes are used when `default` is a `Module`. - `warn_default::Bool = true`: if `true`, throw a warning when a variable does not have an assigned process but it has a default value so that it becomes a parameter instead. - `check_rhs::Bool = true`: if `true`, check that the RHS of all processes is NOT an - `Equation`. Throw an informative error if there is one. This + `Equation` type. Throw an informative error if there is one. This helps scenarios where the RHS is wrongly an `Equation` assigning the LHS itself (has happened to me many times!). """ @@ -162,7 +162,7 @@ end function check_rhs_validity(processes) for p in processes - if rhs(p) <: Equation + if rhs(p) isa Equation lvar = lhs_variable(p) throw(ArgumentError("Process assigned to variable $(lvar) is ill defined. "* "The RHS is an `<: Equation` type but it shouldn't be." From 0051baa8819ecee0bffcf2690b1e424e29165147 Mon Sep 17 00:00:00 2001 From: Datseris Date: Wed, 30 Apr 2025 14:25:09 +0100 Subject: [PATCH 03/10] use new ci file which will hopefully work? --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6278b77..0968dec 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,7 +34,7 @@ jobs: with: version: ${{ matrix.version }} arch: ${{ matrix.arch }} - - uses: actions/cache@v1 + - uses: actions/cache@v4 env: cache-name: cache-artifacts with: From e231e13978cb004a85e7577d304ed2ab55bf3ed1 Mon Sep 17 00:00:00 2001 From: Datseris Date: Wed, 30 Apr 2025 14:45:11 +0100 Subject: [PATCH 04/10] simlper ode test dependency --- test/Project.toml | 2 +- test/runtests.jl | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/Project.toml b/test/Project.toml index d170eec..e8d6433 100644 --- a/test/Project.toml +++ b/test/Project.toml @@ -1,6 +1,6 @@ [deps] LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" -OrdinaryDiffEq = "1dea7af3-3e70-54e6-95c3-0bf5283fa5ed" +OrdinaryDiffEqDefault = "50262376-6c5a-4cf5-baba-aaf4f84d72d7" Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c" Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2" Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" diff --git a/test/runtests.jl b/test/runtests.jl index ef66916..f78c33a 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -1,6 +1,6 @@ using ProcessBasedModelling using Test -using OrdinaryDiffEq +using OrdinaryDiffEqDefault @testset "construction + evolution" begin # The model, as defined below, is bistable due to ice albedo feedback @@ -61,7 +61,7 @@ using OrdinaryDiffEq ufs = [] for u0 in u0s p = ODEProblem(sys, u0, (0.0, 1000.0*365*24*60*60.0)) - sol = solve(p, Tsit5()) + sol = solve(p, Tsit5(); abstol = 1e-9, reltol = 1e-9) push!(ufs, sol.u[end]) end From 92b4121907ded747bf457e26c90fc5b800cad6cc Mon Sep 17 00:00:00 2001 From: Datseris Date: Thu, 1 May 2025 09:16:30 +0100 Subject: [PATCH 05/10] change ODE to Tsit4 --- test/Project.toml | 2 +- test/runtests.jl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Project.toml b/test/Project.toml index e8d6433..7fac242 100644 --- a/test/Project.toml +++ b/test/Project.toml @@ -1,6 +1,6 @@ [deps] LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" -OrdinaryDiffEqDefault = "50262376-6c5a-4cf5-baba-aaf4f84d72d7" +OrdinaryDiffEqTsit5 = "b1df2697-797e-41e3-8120-5422d3b24e4a" Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c" Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2" Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" diff --git a/test/runtests.jl b/test/runtests.jl index f78c33a..686df10 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -1,6 +1,6 @@ using ProcessBasedModelling using Test -using OrdinaryDiffEqDefault +using OrdinaryDiffEqTsit5 @testset "construction + evolution" begin # The model, as defined below, is bistable due to ice albedo feedback From 6bc24ad41830245c417aeb5ce20a6f6d304ab7d2 Mon Sep 17 00:00:00 2001 From: Datseris Date: Thu, 1 May 2025 09:17:08 +0100 Subject: [PATCH 06/10] only check RHS when keyword true --- src/make.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/make.jl b/src/make.jl index b843f6b..33c5327 100644 --- a/src/make.jl +++ b/src/make.jl @@ -86,7 +86,7 @@ function processes_to_mtkeqs(_processes::Vector, default::Dict{Num, Any}; check_rhs = true, ) processes = expand_multi_processes(_processes) - check_rhs_validity(processes) + check_rhs && check_rhs_validity(processes) # Setup: obtain lhs-variables so we can track new variables that are not # in this vector. The vector has to be of type `Num` lhs_vars = Num[lhs_variable(p) for p in processes] From 236e7a58b3883e07b2fd3bd37dba63030c00c874 Mon Sep 17 00:00:00 2001 From: Datseris Date: Thu, 1 May 2025 09:18:46 +0100 Subject: [PATCH 07/10] remove obsolete arguments from mtkeqs function --- src/make.jl | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/make.jl b/src/make.jl index 33c5327..ecedc4e 100644 --- a/src/make.jl +++ b/src/make.jl @@ -82,8 +82,7 @@ processes_to_mtkeqs(procs, default_dict(v); kw...) # The main implementation has the defaults to be a map from variable to process # because this simplifies a bit the code function processes_to_mtkeqs(_processes::Vector, default::Dict{Num, Any}; - type = ODESystem, name = nameof(type), independent = t, warn_default::Bool = true, - check_rhs = true, + warn_default::Bool = true, check_rhs::Bool = true, ) processes = expand_multi_processes(_processes) check_rhs && check_rhs_validity(processes) From 1d901db24e734ee14211a833e584a54192f8d63f Mon Sep 17 00:00:00 2001 From: Datseris Date: Thu, 1 May 2025 09:21:14 +0100 Subject: [PATCH 08/10] improve central docstring --- src/make.jl | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/make.jl b/src/make.jl index ecedc4e..f8ff11e 100644 --- a/src/make.jl +++ b/src/make.jl @@ -8,22 +8,22 @@ passed to the MTK model/system like `ODESystem`. During construction, the following automations improve user experience: -- Variable(s) introduced in `processes` that does not itself have a process obtain +- Variable(s) introduced in `processes` that does not themselves have a process obtain a default process from `default`. -- If no default exists, but the variable(s) itself has a default numerical value, - a [`ParameterProcess`](@ref) is created for said variable and a warning is thrown. +- If no default exists, but the variable(s) have a default numerical value, + a [`ParameterProcess`](@ref) is created for said variable(s) and a warning is thrown. - Else, an informative error is thrown. - An error is also thrown if any variable has two or more processes assigned to it. `processes` is a `Vector` whose elements can be: -1. Any instance of a subtype of [`Process`](@ref). `Process` is a +1. Any instance of a subtype of [`Process`](@ref). `Process` is like a wrapper around `Equation` that provides some conveniences, e.g., handling of timescales or not having limitations on the left-hand-side (LHS) form. 1. An `Equation`. The LHS format of the equation is limited. Let `x` be a `@variable` and `p` be a `@parameter`. Then, the LHS can only be one of: - `x`, `Differential(t)(x)`, `Differential(t)(x)*p`, `p*Differential(t)(x)`, - however, the versions with `p` may fail unexpectedly. Anything else will error. + `x`, `Differential(t)(x)`, `Differential(t)(x)*p`, or `p*Differential(t)(x)`. + The versions with `p` may fail unexpectedly. Anything else will error. 2. A `Vector` of the above two, which is then expanded. This allows the convenience of functions representing a physical process that may require many equations to be defined (because e.g., they may introduce more variables). @@ -43,7 +43,7 @@ modelling libraries based on ProcessBasedModelling.jl is to define modules/submo that offer a pool of pre-defined variables and processes. Modules may register their own default processes via the function [`register_default_process!`](@ref). -These registered processes are used when `default` is a `Module`. +These registered default processes are used when `default` is a `Module`. ## Keyword arguments From 8db3884353ee1accb3e0a74753a0bf4cfb38c148 Mon Sep 17 00:00:00 2001 From: Datseris Date: Thu, 1 May 2025 09:33:01 +0100 Subject: [PATCH 09/10] wrap all tests in a testset --- test/runtests.jl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/runtests.jl b/test/runtests.jl index 686df10..c988698 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -1,7 +1,7 @@ using ProcessBasedModelling using Test using OrdinaryDiffEqTsit5 - +@testset "ProcessBasedModelling" begin @testset "construction + evolution" begin # The model, as defined below, is bistable due to ice albedo feedback # so two initial conditions should go to two attractors @@ -247,3 +247,6 @@ end @test has_symbolic_var(mtk, z) @test has_symbolic_var(mtk, TestDefault.x) end + + +end # @testset \ No newline at end of file From 3f699fed695286f232de954aeb4b58261ecb5683 Mon Sep 17 00:00:00 2001 From: Datseris Date: Thu, 1 May 2025 09:51:54 +0100 Subject: [PATCH 10/10] module outside tests --- test/runtests.jl | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/runtests.jl b/test/runtests.jl index c988698..eabeaae 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -1,6 +1,17 @@ using ProcessBasedModelling using Test using OrdinaryDiffEqTsit5 + +# This module is used in one of the tests +module TestDefault + using ProcessBasedModelling + @variables x(t) = 0.5 y(t) = 0.2 + register_default_process!.([ + Differential(t)(x) ~ 0.2y - x, + y ~ x^2, + ], Ref(TestDefault)) +end + @testset "ProcessBasedModelling" begin @testset "construction + evolution" begin # The model, as defined below, is bistable due to ice albedo feedback @@ -227,17 +238,6 @@ end @test_throws ["an `<: Equation` type"] processes_to_mtkeqs(procs) end - - -module TestDefault - using ProcessBasedModelling - @variables x(t) = 0.5 y(t) = 0.2 - register_default_process!.([ - Differential(t)(x) ~ 0.2y - x, - y ~ x^2, - ], Ref(TestDefault)) -end - @testset "registering default" begin using .TestDefault @variables z(t) = 0.1