Throw an error if structural simplification is applied twice #3027
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| @named rc_model = ODESystem(rc_eqs, t; systems) | ||
| sys = structural_simplify(rc_model) | ||
| @test_throws ModelingToolkit.RepeatedStructuralSimplificationError structural_simplify(sys) |
There was a problem hiding this comment.
[JuliaFormatter] reported by reviewdog 🐶
| @test_throws ModelingToolkit.RepeatedStructuralSimplificationError structural_simplify(sys) | |
| @test_throws ModelingToolkit.RepeatedStructuralSimplificationError structural_simplify(sys) |
| @@ -12,3 +12,6 @@ end | |||
| @safetestset "Bareiss" begin | |||
| include("bareiss.jl") | |||
| end | |||
| @safetestset "Errors" begin | |||
| include("errors.jl") | |||
| end | |||
There was a problem hiding this comment.
[JuliaFormatter] reported by reviewdog 🐶
| end | |
| end |
ChrisRackauckas
force-pushed
the
double_ss_error
branch
from
305bdd2 to
a24ff12
Compare
This is a bug in the tests that was found by SciML/ModelingToolkit.jl#3027
This error found more bad tests than I would have expected!
|
@YingboMa that failing test... doesn't make much sense? |
ChrisRackauckas
force-pushed
the
double_ss_error
branch
from
bdc0c2c to
73054c4
Compare
|
Since downstreams all seem fine other than test bugs, this is good to go. I'm sure someone will complain about this one, but this is pretty much required in order to make sure no weird hidden bugs show up. Calling a system complete means you have completed the system: if you try to change it after you say you will never change it (i.e. that it's complete), bad things will happen. We have to assume users don't lie about when their system is completed or else we could never optimize things like schedules. So... this error is pretty necessary, and if you were violating it there's issues you could have hit before which is why it was made explicit. |
|
I'll follow up with SBMLToolkit which is over zealous at calling |
yes I will, this was quite breaking. Calling |
|
|
| @@ -21,6 +29,7 @@ function structural_simplify( | |||
| sys::AbstractSystem, io = nothing; simplify = false, split = true, | |||
| allow_symbolic = false, allow_parameter = true, conservative = false, fully_determined = true, | |||
| kwargs...) | |||
| iscomplete(sys) && throw(RepeatedStructuralSimplificationError()) | |||
There was a problem hiding this comment.
This is the wrong check and breaking. You should just check the existence of schedule.
Fixes #3012