Throw an error if structural simplification is applied twice (take 2) #3035

YingboMa · 2024-09-09T15:55:00Z

Reverts #3027 Fixes #3012

ChrisRackauckas · 2024-09-09T17:05:14Z

It doesn't make sense to have completion not mean that the system is complete. If you want to make that be a V10 then make it a v10, but we shouldn't just have it be a random name that doesn't match what it's doing

ChrisRackauckas · 2024-09-09T17:08:17Z

.github/workflows/Tests.yml

@@ -28,8 +28,6 @@ jobs:
        group:
          - InterfaceI
          - InterfaceII
-          - SymbolicIndexingInterface


Why is this removed?

It's just reverted together. I can add them back.

You forgot to not delete the tests...

ChrisRackauckas · 2024-09-09T17:10:03Z

test/nonlinearsystem.jl

@@ -251,6 +251,7 @@ end
        0 ~ z - cos(x),
        0 ~ x * y]
    @named ns = NonlinearSystem(eqs, [x, y, z], [])
+    ns = complete(ns)


ChrisRackauckas · 2024-09-09T17:10:17Z

test/mtkparameters.jl

@@ -158,8 +158,8 @@ function level2()
    eqs = [D(x) ~ p1 * x - p23[1] * x * y
           D(y) ~ -p23[2] * y + p4 * x * y]

-    sys = structural_simplify(ODESystem(
-        eqs, t, tspan = (0, 3.0), name = :sys, parameter_dependencies = [y0 => 2p4]))
+    sys = structural_simplify(complete(ODESystem(


Why is this necessary?

That's the whole point of complete. People can write sys = complete(sys) so that namespacing just works out for plot(sol, idxs=sys.v) etc.

But they don't use the indexing, and they immediately call structural simplify here which would set complete itself

I am just saying calling complete before is valid use and should be tested.

ChrisRackauckas · 2024-09-09T17:10:47Z

test/runtests.jl

@@ -32,62 +32,57 @@ end
            @safetestset "Dynamic Quantities Test" include("dq_units.jl")
            @safetestset "Unitful Quantities Test" include("units.jl")
            @safetestset "Mass Matrix Test" include("mass_matrix.jl")
+            @safetestset "SteadyStateSystem Test" include("steadystatesystems.jl")


Why are the tests moved back?

The change just got reverted with your PR.

ChrisRackauckas · 2024-09-09T17:11:06Z

test/structural_transformation/runtests.jl

@@ -12,6 +12,3 @@ end
 @safetestset "Bareiss" begin
    include("bareiss.jl")
 end
-@safetestset "Errors" begin


Why was this test deleted?

You added these changes in the #3027 PR and just reverted together.

Why would you revert this change though? I don't see why you wouldn't want a test.

It's tested https://github.com/SciML/ModelingToolkit.jl/pull/3035/files#diff-c3583950ad88959d447ae8d33ebcfc77d106d9d6a7af0cd394bd8fe3cb1e10d5R47

test/symbolic_events.jl

YingboMa · 2024-09-09T17:15:02Z

Your PR is actually breaking though. And complete has a documented meaning that doesn't mean what you think it means.

YingboMa · 2024-09-09T17:19:40Z

It doesn't make sense to have completion not mean that the system is complete. If you want to make that be a V10 then make it a v10, but we shouldn't just have it be a random name that doesn't match what it's doing

Let's actually read the docs:

help?> ModelingToolkit.complete
  complete(sys::ModelingToolkit.AbstractSystem; split) -> Any


  Mark a system as completed. If a system is complete, the system will no longer namespace its
  subsystems or variables, i.e. isequal(complete(sys).v.i, v.i).

complete is defined to just mean not namespacing the toplevel.

ChrisRackauckas · 2024-09-09T17:20:55Z

Well it says two things, it says the model is complete and it says that the indexing is changed. If you want, we can fix this with a v10, but having the iscomplete not actually mean the model is complete is not a good idea.

YingboMa · 2024-09-09T17:23:09Z

iscomplete just means no more namespacing? What else does it mean? And for whatever definition you can come up with, it sure doesn't mean simplified either. This is not a breaking change.

ChrisRackauckas · 2024-09-09T17:26:04Z

If it's complete then ... . It's not saying a model is complete if and only if it indexes like that. The if only defines one extra property. It marks that the model was completed, and it also signifies that it will change its indexing behavior.

And it wouldn't make sense for it to be the iff either: if I was to say "I have completed building this model" and you had never used MTK, would your first though be "so therefore it drops the top level scope"? No one would guess complete means that, so if this primitive is supposed to mean that, it should really be deprecated to a different name.

But at the same time, we need a marker for completeness since other optimizations in the near future will require knowing that property, so we still need something for "this model has been completed"

YingboMa · 2024-09-09T17:27:33Z

  Mark a system as completed. If a system is complete, the system will no longer namespace its
  subsystems or variables, i.e. isequal(complete(sys).v.i, v.i).

This is the documented definition of complete. No? So it's not breaking period.

YingboMa · 2024-09-09T17:30:10Z

The definition of complete is moot anyway. Your PR is actually breaking because

sys = complete(sys)
s = structural_simplify(sys)

is a common and valid use of complete. We have to revert #3027. And all this PR does is reverting the PR and fix #3012 properly. complete is the wrong check for simplified.

ChrisRackauckas · 2024-09-09T17:31:19Z

Let's just change the words a bit and see if the logic makes sense.

Doc string: issquare. Marks that the object is a square. If the object is a square then it is red.

Is it breaking to think that issquare is true means that the object is a square? Or can we only assume that something that issquare is red? If that's the case, why is it not called isred?

ChrisRackauckas · 2024-09-09T17:33:42Z

If you're going to revert then, there's no reason to revert the other test changes. Can we just do this right instead?

YingboMa · 2024-09-09T17:34:19Z

Is the definition of complete relevant in the context of this particular PR?

Throw an error if structural simplification is applied twice #3027 is VERY breaking because

sys = complete(sys)
s = structural_simplify(sys)

is a common and valid pattern, and 3027 breaks that.
2. isscheduled is the proper check for if a system is simplified and complete is separate property.

YingboMa · 2024-09-09T17:42:16Z

You are making complete sound like an unreasonable and ridiculous definition.

I defined complete the way it is because from a modeler's perspective, a model is complete when he or she no longer needs to namespace the toplevel model. sys.v needs namespacing during model building, but once the building process is complete sys.v should no longer namespace so that sol(t, idxs=sys.v) just works as intended.

ChrisRackauckas · 2024-09-09T17:42:30Z

Okay, then what about yanking, fixing the definitions and picking good descriptive names, and doing a v10?

YingboMa · 2024-09-09T17:45:30Z

I think complete is a descriptive name for a modeler and a compiler person should use the isscheduled check most of the time.

ChrisRackauckas · 2024-09-09T17:48:41Z

I defined complete the way it is because from a modeler's perspective, a model is complete when he or she no longer needs to namespace the toplevel model

So you think if we went to the modelica conference and asked people what it meant for a model to be complete is, you think that is the definition they would give? You don't think they would say that it meant they were done changing the model?

I get that we read the docstring differently. And I see how you could have meant it to mean something different. But let's take a step back here, it's getting emotional. I really don't think that is what most people would expect complete to mean, we will need a sense of completion for other optimizations we add in the near future to be valid, and we can make things better. How about picking more clear words, splitting these meanings, and doing a v10?

It also doesn't make sense that top level indexing changes are one way. Why not allow for flipping that on and off? Tying it to a form of finalization and making it a one way operation is the problem some others are having with ergonomics. If we split this idea from completion, then we can make it swappable and fix other ergonomics issues Ben was having. That seems like a good reason to take the moment to do the real fix and v10

YingboMa · 2024-09-09T17:52:15Z

Yeah, complete means a modeler will no longer change the model, which programmatically, just means toplevel will no longer namespace.

It still doesn't mean iscomplete implies the system is simplified. #3027 should use isscheduled instead. So either way, we don't need a V10 to merge this PR.

ChrisRackauckas · 2024-09-09T17:54:25Z

Yeah, complete means a modeler will no longer change the model, but it doesn't mean iscomplete implies the system is simplified.

Yes, it doesn't necessarily mean that a model is simplified, but if it means "a modeler will no longer change the model", shouldn't you error in structural simplify since they are now changing the model?

YingboMa · 2024-09-09T17:55:27Z

Structural simplify is not stateful. It just returns another object, so they are not changing the model.

baggepinnen · 2024-09-09T17:58:26Z

So you think if we went to the modelica conference and asked people what it meant for a model to be complete is, you think that is the definition they would give? You don't think they would say that it meant they were done changing the model?

A modelica person typically does not invoke structural simplification themselves, they press play and the tool performs translation and simulation, so I'm pretty sure they consider the model complete when they hand it off to the tool to do it's thing. FWIW, I completely agree with Yingbo in this matter

ChrisRackauckas · 2024-09-09T18:02:52Z

If complete is about symbolic indexing and the numerical function building codes don't use the symbolic indexing, why error if calling ODEProblem on a model that isn't complete? If that's what it means then it doesn't make sense to mix it with that.

YingboMa · 2024-09-09T18:06:04Z

It should error because then the sys in a problem isn't complete from a modeler's perspective, and not ready to simulate.

Programmatically, it means sol(t, idxs=sys.v) or anything that takes sys.v will not work as intended.

ChrisRackauckas · 2024-09-09T18:13:09Z

Okay, I think I am seeing a different definition from you now:

Complete does mean that the modeler won't mutate the model.
Running structural simplify on a completed model is okay because it's out of place and does not mutate the model.
Checking for the schedule is required because you cannot simplify twice.

Is that correct?

So okay, I am fine with this definition of "cannot change the model" to mean "cannot mutate the model", and thus structural simplification is fine. If this is the case:

Change "has schedule" to "already simplified"? It would be generally easier to understand, through I do see the edge case that technically you can simplify again if you drop the schedule. If has schedule is kept, it should have a clear doc string saying that it comes from structural simplification.
Clarify the iscomplete docstring that it's expected the model won't be mutated after set.

YingboMa · 2024-09-09T18:21:38Z

Okay, I think I am seeing a different definition from you now: ...

I maintain the definition that complete means the modeler will no longer build on top of the model using MTK's frontend, i.e. equivalent to not namespacing at toplevel. Mutability doesn't have anything to do with complete. Of course, mutating a system is generally bad but not just because a system is completed. We should document that mutation on any system is discouraged because of aliasing issues.

Normally, isscheduled is the thing that matters. If a system doesn't have a schedule object, then chaining structural simplification arbitrary number of times is still valid. In this PR though, I am being very conservative and implemented isscheduled as simplified, but a more precise implementation is possible, i.e. just isscheduled(s) = has_schedule(s) && get_schedule(s) !== nothing.

ChrisRackauckas · 2024-09-09T18:28:38Z

Let's be more precise.

With that definition of complete, we should validate that there are no unbound inputs (with a keyword arg to turn off the check) and check units at that point too?

YingboMa · 2024-09-09T18:30:10Z

We could if we want but it has nothing to do with this PR being merged as a fix for MTK v9.

YingboMa · 2024-09-09T21:54:30Z

Since this PR is a non-breaking strict improvement compare to the broken state right now, I will merge and tag a new release.

ChrisRackauckas · 2024-09-09T22:15:05Z

This shouldn't've merged without fixing the tests though...

#3035 accidentally reverted fixes to the test setup and checks that the error is thrown

YingboMa added 2 commits September 9, 2024 11:54

Revert "Throw an error if structural simplification is applied twice"

e9617e5

Throw an error if structural simplification is applied twice

49b5c3f

YingboMa changed the title ~~Revert "Throw an error if structural simplification is applied twice"~~ Throw an error if structural simplification is applied twice (take 2) Sep 9, 2024

YingboMa added 2 commits September 9, 2024 12:45

Add docs

1893768

Fix tests

d1dbfcc

YingboMa force-pushed the revert-3027-double_ss_error branch from 0c63790 to d1dbfcc Compare September 9, 2024 17:04

ChrisRackauckas requested changes Sep 9, 2024

View reviewed changes

YingboMa merged commit 4f2385e into master Sep 9, 2024
22 of 24 checks passed

YingboMa deleted the revert-3027-double_ss_error branch September 9, 2024 21:54

ChrisRackauckas added a commit that referenced this pull request Sep 9, 2024

Fix test setup

717660e

#3035 accidentally reverted fixes to the test setup and checks that the error is thrown

ChrisRackauckas mentioned this pull request Sep 9, 2024

Fix test setup #3038

Merged

Throw an error if structural simplification is applied twice (take 2) #3035

Throw an error if structural simplification is applied twice (take 2) #3035

Conversation

YingboMa commented Sep 9, 2024 • edited Loading

ChrisRackauckas commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YingboMa commented Sep 9, 2024

YingboMa commented Sep 9, 2024

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024 • edited Loading

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024

YingboMa commented Sep 9, 2024 • edited Loading

ChrisRackauckas commented Sep 9, 2024

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024 • edited Loading

YingboMa commented Sep 9, 2024

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024 • edited Loading

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024

baggepinnen commented Sep 9, 2024

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024 • edited Loading

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024 • edited Loading

YingboMa commented Sep 9, 2024 • edited Loading

ChrisRackauckas commented Sep 9, 2024

YingboMa commented Sep 9, 2024 •

edited

Loading

YingboMa commented Sep 9, 2024 •

edited

Loading

YingboMa commented Sep 9, 2024 •

edited

Loading

YingboMa commented Sep 9, 2024 •

edited

Loading

YingboMa commented Sep 9, 2024 •

edited

Loading

YingboMa commented Sep 9, 2024 •

edited

Loading

YingboMa commented Sep 9, 2024 •

edited

Loading

YingboMa commented Sep 9, 2024 •

edited

Loading