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

`@extend` does not work with parameters #3061

Open baggepinnen opened 1 month ago

baggepinnen commented 1 month ago

When using @mtkmodel and @extend, the constructor does not know about the parameters of the extended system. MWE:

@mtkmodel Inner begin
    @parameters begin
        color[1:4] = [1.0, 0.0, 0.0, 1.0], [description = "Color of the body in animations"]
        render = true, [description = "Render the joint in animations"]
    end
end

@mtkmodel Outer begin
    @extend () = v = Inner()
    @parameters begin
        radius = 0.1, [description = "Radius of the sphere in animations"]
    end
end

@named test = Outer(color=[1,1,1,1])
julia> @named test = Outer(color=[1,1,1,1])
ERROR: MethodError: no method matching __Outer__(; name::Symbol, color::Vector{Int64})

Closest candidates are:
  __Outer__(; name, radius) got unsupported keyword argument "color"
   @ Main ~/.julia/packages/ModelingToolkit/UXr3S/src/systems/model_parsing.jl:138
ven-k commented 1 month ago

One must list all the args (parameter or variable), of a sub-component or a base-system, that should be added to the arglist of the main-component.

This will fix the issue:

@mtkmodel Outer begin
     @extend () = v = Inner(; color, render)
     @parameters begin
         radius = 0.1, [description = "Radius of the sphere in animations"]
     end
 end
baggepinnen commented 1 month ago

The component that is extended may have 10s-100s of parameters and variables, if we have to type them all out it kind-of defeats the purpose of extending something in the first place?

ven-k commented 1 month ago

It was mainly to keep the syntax same in both cases. If it is more convenient, I can modify the case for extend to automatically add all args.

baggepinnen commented 1 month ago

I can modify the case for extend to automatically add all args.

That sounds good to me, and aligns with what I would expect from inheritance

hexaeder commented 1 month ago

The problem with adding the args explicitly after extending is that they are not optional anymore. You always have to provide color argument, even though Inner has defaults defined for it.

baggepinnen commented 1 month ago

That might be a problem with the current implementation, it does not mean that we can't change the implementation to something better?

ven-k commented 1 month ago

Note that, even now mtkmodel handles it gracefully. It adds nothing as default value and internally checks if input is provided and updates suitably. So you would not have to always provide an input.

Extending all args to main component automatically, will also handle it the same way; all args will be available but remain optional. (I expect this to land soon)

wang890 commented 1 month ago

It was mainly to keep the syntax same in both cases. If it is more convenient, I can modify the case for extend to automatically add all args.

Good.

Another @extendissue, please see #2758: Still in current version v9.46.1, when @extend multiple times, only the last one takes effect, that is, only the last extended component in the :extend field of ModelingToolkit.Model Thanks @ven-k