[email protected] broke MTK

[hexaeder]

I understand that #1351 needed to be fixed. But changing the semantics of an exported function in ModelingToolkit is quite a bad breakage, in my opinion. Well, that ship has sailed, so feel free to just close this issue. But I wanted to post it anyway to raise awareness that a PR was not considered breaking even though it explicitly aimed at changing the output of an exported function for the same input, which is like the definition of breaking.

using Pkg
pkg"activate --temp"
pkg"add [email protected], [email protected]"
using Symbolics, ModelingToolkit
using ModelingToolkit: D_nounits as D, t_nounits as t
@mtkmodel Example begin
    @variables begin
        x(t)
        y(t)
    end
    @equations begin
        x ~ y
    end
end
@named ex = Example()
get_variables(only(equations(ex)))
# 2-element Vector{SymbolicUtils.BasicSymbolic}:
#  x(t)
#  y(t)

using Pkg
pkg"activate --temp"
pkg"add [email protected], [email protected]"
using Symbolics, ModelingToolkit
using ModelingToolkit: D_nounits as D, t_nounits as t
@mtkmodel Example begin
    @variables begin
        x(t)
        y(t)
    end
    @equations begin
        x ~ y
    end
end
@named ex = Example()
get_variables(only(equations(ex)))
# 3-element Vector{SymbolicUtils.BasicSymbolic}:
#  x
#  t
#  y

[hexaeder]

After getting rid of my mildly annoyed state by posting this issue, curiosity took over. I don't actually get it, why does the following code behave so differently between an mtk equation and a "pure" equation? 🤔

using Pkg
pkg"activate --temp"
pkg"add [email protected], [email protected]"
using Symbolics, ModelingToolkit

@independent_variables t
@variables x(t) y(t);
get_variables(x ~ y)
# 2-element Vector{SymbolicUtils.BasicSymbolic}:
#  x(t)
#  y(t)

@mtkmodel Example begin
    @variables begin
        x(t)
        y(t)
    end
    @equations begin
        x ~ y
    end
end
@named ex = Example()
get_variables(only(equations(ex)))
# 3-element Vector{SymbolicUtils.BasicSymbolic}:
#  x
#  t
#  y

[ChrisRackauckas]

I agree these should probably just be two different functions. get_variables vs get_leaf_variables, or a keyword argument return_leaves = true? I can see the need for both options, so it's not a one or the other kind of thing.

One of the things the tests didn't catch though is I would've assumed the result was [x(t), y(t), t], dropping the dependencies seems a bit odd to me. Worth an option too though if that's what the codegen needs?

@AayushSabharwal

[AayushSabharwal]

even though it explicitly aimed at changing the output of an exported function for the same input, which is like the definition of breaking.

MTK needs to change the output. With a callable parameter @parameters p(..), and @variables x(t), MTK was unable to figure out that p(x) depends on x because get_variables didn't look inside. It's weird that MTK also returns t in the MWE. My guess is @mtkmodel is doing @variables x(..); x = x(t) for some reason.

[AayushSabharwal]

One of the things the tests didn't catch though is I would've assumed the result was [x(t), y(t), t], dropping the dependencies seems a bit odd to me. Worth an option too though if that's what the codegen needs?

Apparently it never returned t which I agree is weird. I don't think it's a codegen issue, it's more of structural_simplify needing information.

[ChrisRackauckas]

What about adding the kwarg?

[hexaeder]

MTK needs to change the output.

I believe that it was for a good reason, and I don't mind the breakage. I only mind if it happens in a minor version. Feel free releasing breaking versions of MTK twice a weak if it helps developing the project.

However, I don't want to dwell on that; things happen. On the concrete matter, I'd like to understand what the new desired behavior is to fix my code. Looking at the this example again I am getting mixed signals because two very similar calls behave completely different:

using Pkg
pkg"activate --temp"
pkg"add [email protected], [email protected]"
using Symbolics, ModelingToolkit

@independent_variables t
@variables x(t) y(t);
get_variables(x ~ y)
# 2-element Vector{SymbolicUtils.BasicSymbolic}:
#  x(t)
#  y(t)

@mtkmodel Example begin
    @variables begin
        x(t)
        y(t)
    end
    @equations begin
        x ~ y
    end
end
@named ex = Example()
get_variables(only(equations(ex)))
# 3-element Vector{SymbolicUtils.BasicSymbolic}:
#  x
#  t
#  y

To me, both look kinda broken since I'd expect [x(t), y(t), t] in both cases.
I hope returning x and y without independent is just a bug, because the (t) is almost never dropped anywhere in MTK. For example

unknowns(ex) ∩ get_variables(only(equations(ex)))

empty seems undesirable.

---

PS:
The difference seems to have to do with the dsl constructor

@independent_variables t
@variables x(t) y(t);
@named sys = ODESystem([x~y], t, [x, y], [])
get_variables(only(equations(sys)))
# 2-element Vector{SymbolicUtils.BasicSymbolic}:
#  x(t)
#  y(t)

[AayushSabharwal]

Yeah, @mtkmodel is weird. I have a fix for it which returns x(t), y(t) again. Do we want just that? Do we want Symbolics to return to the old behavior and enable the new one with a kwarg? Should we revert Symbolics, and change MTK to not use get_variables since it's inherently relying on behavior specific to MTK (@parameters callables)?

[ChrisRackauckas]

Should we revert Symbolics, and change MTK to not use get_variables since it's inherently relying on behavior specific to MTK (@parameters callables)?

I think so. Though the ability to get [x,y,t] is a nice option to keep and document (though I think you'd probably want [x(t),y(t),t] most of the time). So it should be reverted but have the new options, and MTK should use what it needs but I don't think that should effect the default motion of this function. MTK does need something specific but for Symbolics this is a more general utility.

[AayushSabharwal]

Turns out changing MTK to not use get_variables! is breaking. Catalyst uses it for their reactions here. What if I just fix @mtkmodel?

[ChrisRackauckas]

What I described is the opposite of breaking though?

[AayushSabharwal]

If we revert to the old behavior, then #1351 is still an issue. If we add a keyword to enable the current behavior and make MTK use it, then it's breaking because the method defined in Catalyst doesn't take that keyword so they'd end up with MethodErrors.

[ChrisRackauckas]

Catalyst should update to add the kwargs. You can do a hasmethod to do backwards compat

[AayushSabharwal]

hasmethod will always return true because Symbolics has an (::Any, ::Any, ::Any) method which also needs to accept the kwarg (since otherwise any call to get_variables! would have to check hasmethod).

[isaacsas]

get_variables was never intended to inspect the arguments of symbolic variables. i.e. this was the intended behavior:

julia> using ModelingToolkit, Symbolics

julia> @variables t A(t) B(t)
3-element Vector{Num}:
    t
 A(t)
 B(t)

julia> ex = t*A*B
t*B(t)*A(t)

julia> get_variables(ex)
3-element Vector{SymbolicUtils.BasicSymbolic}:
 t
 B(t)
 A(t)

julia> get_variables(A*B)
2-element Vector{SymbolicUtils.BasicSymbolic}:
 A(t)
 B(t)

allowing one to distinguish an expression that explicitly depends on t from one that doesn't. And this behavior is definitely relied on in places.

It should inspect arguments to external functions, i.e. this works

julia> f(x,y) = x + y
f (generic function with 1 method)

julia> @register_symbolic f(x,y)

julia> get_variables(A*f(B,t))
3-element Vector{SymbolicUtils.BasicSymbolic}:
 A(t)
 B(t)
 t

julia> get_variables(f(B,A*B))
2-element Vector{SymbolicUtils.BasicSymbolic}:
 B(t)
 A(t)

Changing that convention is breaking at this point. When it was created unbound callable variables were never used as far as I am aware (their use is much more recent, so they weren't a consideration in how it should work).

[AayushSabharwal]

That is how it works now, with SciML/ModelingToolkit.jl#3217

[isaacsas]

Great! I just wanted to clarify how it has worked historically since there was a lot of discussion about it not returning t from get_variables(A(t)*B(t)) above, which was an intentional decision and not a bug.

[ChrisRackauckas]

Is this fixed now? I don't recall the revert on the symbolics side now.

[AayushSabharwal]

Symbolics isn't getting a revert, MTK is getting a fix SciML/ModelingToolkit.jl#3217

[ChrisRackauckas]

But wouldn't get_variables still have changed?

[AayushSabharwal]

We're in a bit of a deadlock basically:

• Reverting the get_variables change opens up get_variables doesn't work for function arguments #1351 again
• Reverting and adding a kwarg to enable the new behavior is also breaking. We can't always pass the kwarg, because downstream libraries like catalyst don't define a method with it. We can't hasmethod check the kwarg because there is always an applicable method ([email protected] broke MTK #1364 (comment))
• We can't change MTK to not use get_variables because Catalyst relies on adding methods to it to use the relevant MTK infrastructure

[isaacsas]

If there is a deadlock I would argue for reverting and avoiding having a breaking change in a non-breaking release (which can impact already released versions of downstream dependents that did not anticipate this change).

Perhaps #1351 can be fixed without a breaking MTK release by adding a kwarg to the relevant dependency graph functions that takes an alternative to get_variables and modified_unknowns (but keeps them as the defaults to not be breaking). That would presumably allow the issue there to be fixed by passing an alternative to get_variables that has the desired behavior (perhaps just passing vars as you suggested earlier?).

[isaacsas]

And we are happy to update Catalyst to have the needed kwargs / new API function for V15 if an interface is settled for that in the near future. (But then MTK probably needs a breaking release too for that new version.)

[isaacsas]

With all that said, if I understand right the merged change should only impact callables declared without fixed arguments initially, which shouldn’t impact Catalyst’s existing releases I think (since we never intentionally use them, and only were generating them accidentally for observables).

[AayushSabharwal]

Perhaps #1351 can be fixed without a breaking MTK release by adding a kwarg to the relevant dependency graph functions that takes an alternative to get_variables and modified_unknowns (but keeps them as the defaults to not be breaking). That would presumably allow the issue there to be fixed by passing an alternative to get_variables that has the desired behavior (perhaps just passing vars as you suggested earlier?).

That's a good idea.

(But then MTK probably needs a breaking release too for that new version.)

Yeah, which we can't do.

With all that said, if I understand right the merged change should only impact callables declared without fixed arguments initially, which shouldn’t impact Catalyst’s existing releases I think (since we never intentionally use them, and only were generating them accidentally for observables).

Yeah. The only case this impacts is @variables x(..); x(t). DDEs are such a case, but MTK tests passed so it seemed like that part didn't get affected.