feat!: do not scalarize parameters, fix some tests #2469
Conversation
AayushSabharwal
force-pushed
the
as/no-scalarize
branch
from
368b5e1 to
2447c8d
Compare
AayushSabharwal
force-pushed
the
as/no-scalarize
branch
from
2447c8d to
99b259c
Compare
AayushSabharwal
force-pushed
the
as/no-scalarize
branch
2 times, most recently
from
1b028d5 to
c076e60
Compare
|
format |
AayushSabharwal
force-pushed
the
as/no-scalarize
branch
from
c076e60 to
2a9204f
Compare
|
JuliaFormatter is getting updated a lot recently |
|
I have the same version of JuliaFormatter as CI, but locally it doesn't complain and here it does? |
AayushSabharwal
force-pushed
the
as/no-scalarize
branch
2 times, most recently
from
220d6c3 to
decfb51
Compare
Co-authored-by: Aayush Sabharwal <[email protected]>
ChrisRackauckas
force-pushed
the
as/no-scalarize
branch
from
60396c3 to
812e004
Compare
src/systems/diffeqs/odesystem.jl
Outdated
| @assert all(control -> any(isequal.(control, ps)), controls) "All controls must also be parameters." | ||
|
|
||
| deqs = reduce(vcat, scalarize(deqs); init = Equation[]) |
There was a problem hiding this comment.
We should never scalarize during construction for MTKv9. We should just follow what I did in #2472
There was a problem hiding this comment.
If one wants a scalarized system for simulation, then one needs to call structural_simplify.
| !istree(O) && return vars | ||
| if operation(O) === (getindex) |
There was a problem hiding this comment.
If we want structural_simplify to work correctly, we have to differentiate x and x[1] alone. https://github.com/SciML/ModelingToolkit.jl/pull/2472/files#diff-47c27891e951c8cd946b850dc2df31082624afdf57446c21cb6992f5f4b74aa2R351-R369 has the correct implementation
AayushSabharwal
force-pushed
the
as/no-scalarize
branch
from
a84ee9c to
1275d6e
Compare
|
All tests pass locally 🎉 v9 is good to go (after JuliaSymbolics/Symbolics.jl#1057 is fixed) |
| @@ -232,7 +232,7 @@ end | |||
|
|
|||
| function collect_defaults!(defs, vars) | |||
| for v in vars | |||
| (haskey(defs, v) || !hasdefault(v)) && continue | |||
| (haskey(defs, v) || !hasdefault(unwrap(v))) && continue | |||
There was a problem hiding this comment.
Should v ever be wrapped? It will be a pretty hard to catch bug.
|
We should bound Symbolics to be at least |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.