search

SciML / ModelingToolkit.jl

An acausal modeling framework for automatically parallelized scientific machine learning (SciML) in Julia. A computer algebra system for integrated symbolics for physics-informed machine learning and automated transformations of differential equations
https://mtk.sciml.ai/dev/
Other
1.43k stars 209 forks source link

System transformations sometimes dropping fields #1710

Open isaacsas opened 2 years ago

isaacsas commented 2 years ago

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.

ChrisRackauckas commented 2 years ago

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

bowenszhu commented 2 years ago

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!?