Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot create problems with tuple vector variables #3280

Open
TorkelE opened this issue Dec 19, 2024 · 11 comments
Open

Cannot create problems with tuple vector variables #3280

TorkelE opened this issue Dec 19, 2024 · 11 comments
Labels
bug Something isn't working

Comments

@TorkelE
Copy link
Member

TorkelE commented Dec 19, 2024

using ModelingToolkit, OrdinaryDiffEq
using ModelingToolkit: t_nounits as t, D_nounits as D

@variables (X(t))[1:2]
@parameters p[1:2]
eqs = [
    D(X[1]) ~ p[1] - X[1],
    D(X[2]) ~ p[2] - X[2]
]
@named model = ODESystem(eqs, t)
model = complete(model)

# Problem inputs
u0_vec = [X => [1.0, 2.0]]
u0_dict = Dict([X => [1.0, 2.0]])
u0_tup = (X => [1.0, 2.0])
p_vec = [p => [4.0, 5.0]]
p_dict = Dict([p => [4.0, 5.0]])
p_tup = (p => [4.0, 5.0])
tspan = (0.0, 10.0)

# Problem creation:
ODEProblem(model, u0_vec, tspan, p_vec)
ODEProblem(model, u0_vec, tspan, p_dict)
ODEProblem(model, u0_vec, tspan, p_tup)
ODEProblem(model, u0_dict, tspan, p_vec)
ODEProblem(model, u0_dict, tspan, p_dict)
ODEProblem(model, u0_dict, tspan, p_tup)
ODEProblem(model, u0_tup, tspan, p_vec) # Error.
ODEProblem(model, u0_tup, tspan, p_dict) # Error.
ODEProblem(model, u0_tup, tspan, p_tup) # Error.

Probably holds for other problem types as well

@TorkelE TorkelE added the bug Something isn't working label Dec 19, 2024
@ChrisRackauckas
Copy link
Member

I'm not sure this is supported? Dictionaries and things that transform to dictionaries are supported. Tuples of pairs I guess could work, we'd need to add it to to_dict. What's the use case here?

@TorkelE
Copy link
Member Author

TorkelE commented Dec 20, 2024

I think Tuples have always been supported, right? For parameters I think they are even required for when you have different types in your values. Right now I don't think it is a thing for variables, but seems weird to specifically disallow it here (especially as it might even be useful for hybrid ODE/Jump systems)

@ChrisRackauckas
Copy link
Member

No, the parameters always uses an MTKParameters so it always constructs a different type-stable object and has a representation independent of the user code there. That's a requirement in general in order to support things like non-tunables.

@isaacsas
Copy link
Member

Tuples were supported in MTK as a way to preserve the type of parameters for years. Then a breaking change was made that upconverted everything to floats in a non-breaking MTK8 release (but tuples were still supported as mappings). Catalyst has used tuple mappings forever because we needed mixed parameter types long before MTK9. There doesn’t seem to be any indication that MTK9 dropped tuple mapping support in the NEWS.md?

@ChrisRackauckas
Copy link
Member

It's a part of the change to MTKParameters. But there's no reason to not support it, we might as well just convert it to a vector since that's always faster, and it would be faster for the user to just give a vector but we might as well collect general collections.

@ChrisRackauckas
Copy link
Member

Wait this is for u0 being a tuple? We've never supported that, the differential equation solver needs vector operations in order to work.

@TorkelE
Copy link
Member Author

TorkelE commented Dec 22, 2024

Not sure if they get converted to arrays or something internally, but we probably support hundreds of cases of tuple u0. The only case (which is reported as a bug here) is when one of the values are vector vector-valued

u0_tup = (X => [1.0, 2.0])

E.g. if the values are scalar that has always been fine, and it has worked over all problem types, using defaults, remake, for ensembles etc. etc. It just seems like an annoying omission to specifically disallow it for enclosing vector-valued unknowns.

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Dec 22, 2024

We do not support tuples. We have a specific error message about this and we've had that since 2020, see: SciML/DifferentialEquations.jl#566

The semantics on u0 are supposed to be type-matching: if you use defined u0 as a static vector, then u0 in the problem should be a static vector. With that being the case, and knowing that we do not support u0 as a tuple, what is this supposed to mean? We need to refine the type-matching definition a bit since for example it will vec etc., so it's not completely type-matching, but since v9 I wouldn't know what this would actually mean in a way that shouldn't just error (even for the scalar case: if it's making a vector-valued u0 that's arguably a bug and it should be making a tuple-valued u0 which is then an unsolvable problem).

@TorkelE
Copy link
Member Author

TorkelE commented Dec 22, 2024

I am not sure about DiffEq, but Tuples have worked in MTK since forever, e.g.

using OrdinaryDiffEq, ModelingToolkit
using ModelingToolkit: t_nounits as t, D_nounits as D

@parameters p d
@variables X(t) Y(t)

eqs = [
    D(X) ~ p - d * X,
    D(Y) ~ p - d * Y,
]
@mtkbuild sys = ODESystem(eqs, t)

u0 = (X => 1.0, Y => 2.0)
tspan = (0.0, 100.0)
p = (p => 1.0, d => 2.0)

prob = ODEProblem(sys, u0, tspan, p)
sol = solve(prob)

There are also some test cases: https://github.com/SciML/ModelingToolkit.jl/blob/master/test/sciml_problem_inputs.jl

Like, I don't think u0_tup = (X => [1.0, 2.0]) working or not is a biggie, but it seems easier to just fix that than changing everything else. And it might even come in useful some time.

@ChrisRackauckas
Copy link
Member

Yes but that's clearly a bug though since the u0 type is wrong:

julia> prob = ODEProblem(sys, u0, tspan, p)
ODEProblem with uType Vector{Float64} and tType Float64. In-place: true
timespan: (0.0, 100.0)
u0: 2-element Vector{Float64}:
 1.0
 2.0

some implicit collect is converting the type, so it's non-type preserving. We could just convert to Vector if that's better semantics than erroring, but it's a bit confusing for it to exist given that it requires breaking the standard conventions.

@isaacsas
Copy link
Member

Yeah, my comment above was in reference to parameter tuples. For u0 I agree that this is probably not very useful since it has to convert in the solvers. (I think that at least in documentation I wrote I tried to avoid tuples for u0.)

But it might be better to explicitly disallow it for u0 rather than implicitly convert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants