ModelingToolkit.jl icon indicating copy to clipboard operation
ModelingToolkit.jl copied to clipboard

System transformations sometimes dropping fields

Open isaacsas opened this issue 3 years ago • 2 comments

This is something that came up as I was looking at adding events into other system types. Maybe this isn't an easy fix, or desired, but right now when new fields get added to systems they don't always get accounted for in all places systems are transformed. For example,

https://github.com/SciML/ModelingToolkit.jl/blob/9c0d0437e300dab6d50ea1f7369e554e66f41b45/src/systems/diffeqs/odesystem.jl#L377

drops fields like observed, as does

https://github.com/SciML/ModelingToolkit.jl/blob/9c0d0437e300dab6d50ea1f7369e554e66f41b45/src/systems/diffeqs/sdesystem.jl#L164

and

https://github.com/SciML/ModelingToolkit.jl/blob/9c0d0437e300dab6d50ea1f7369e554e66f41b45/src/systems/diffeqs/sdesystem.jl#L261

It seems like there is a need for a transformation/copying mechanism where one can just specify values for sub-fields that have changed, but the remaining fields get automatically propagated.

I guess this could potentially introduce silent bugs by incorrectly propagating fields that actually require transformations, as opposed to now where there are silent bugs with fields being dropped. Maybe the copying mechanism needs to require that all fields be explicitly passed in unless one passes a flag that default values can be propagated for non-selected fields (forcing one to make a choice to propagate the remaining values unchanged).

It does seem though like it would be helpful to have such a functionality, since system transformations that only modify a few fields crop up all over the place.

isaacsas avatar Jul 30 '22 19:07 isaacsas

Agreed on this. Some kind of remake functionality could improve the robustness of pass writing.

ChrisRackauckas avatar Aug 07 '22 23:08 ChrisRackauckas

The transformation methods mentioned in the PR description all create a new system by calling the corresponding constructor, while some other methods, for example alias_elimination and ode_order_lowering, make use of @set! from Setfield.jl, which safely keep the original fileds including observed, defaults, continuous_events.

https://github.com/SciML/ModelingToolkit.jl/blob/a09b9140eff8ebd87dfbbc78a1a92037fc2ab7a8/src/systems/diffeqs/first_order_transform.jl#L7-L13

https://github.com/SciML/ModelingToolkit.jl/blob/9c0d0437e300dab6d50ea1f7369e554e66f41b45/src/systems/alias_elimination.jl#L88-L93

https://github.com/SciML/ModelingToolkit.jl/blob/9c0d0437e300dab6d50ea1f7369e554e66f41b45/src/systems/connectors.jl#L308-L314

Calling constructors and manually copy-pasting fields in each transformation method is a bit cumbersome. Can we change all of them to use Setfield.@set!?

bowenszhu avatar Aug 10 '22 23:08 bowenszhu