Skip to content

Conversation

@cstjean
Copy link
Contributor

@cstjean cstjean commented Nov 21, 2025

This PR rewrites the @components parsing logic. Instead of parsing the RHS of each equation in the @components block, it evaluates it normally as plain julia code. Because the new code is a lot simpler, it was relatively easy to improve the flexibility of the model construction interface (see below) along the lines of #3791

There's still a lot of work to do, so I'd like to get some feedback before pushing forward (... or giving up). Is that a reasonable direction to take?

The good

  • Net reduction in model-parsing code {3FF2A5A2-A748-4C2F-B166-13A135C030A7}
  • Can now change structural parameters with __ syntax: @mtkcompile my_model = MyModel(sub_component__N=10)
  • Can now change subcomponents with __ syntax: @mtkcompile my_model = MyModel(sub_component__subsub_component=MyAlternativeComponent()). I believe this fixes Replacing subsystems #2038.
  • No parsing on the RHS of the components means that the RHS can be arbitrary Julia code, which is a lot more intuitive and general. For example,
    my_interpolation = LinearInterpolation(df.data, df.time)
    @mtkmodel MassSpringDamperSystem2 begin
        @components begin
            src = Interpolation(itp=my_interpolation)
    can now be
    @mtkmodel MassSpringDamperSystem2 begin
        @components begin
            src = Interpolation(LinearInterpolation, df.data, df.time)
  • This also supports heterogeneous arrays, as in resistors = [FixedResistor(), VariableResistor()].
  • Nested if blocks now work inside of @components

The bad

Model metadata for components @test A.structure[:components] == [[:cc, :C]] now yields [[:cc, :unimplemented]]. (But System metadata is fine). How bad is that? Where is that information used? Maybe Dyad?

Potential workaround: bring back #master's parsing code just for metadata parsing. Maybe if I strip that code of everything except the metadata part, it would be reasonably small, and we can give up on more complex cases (that aren't supported ATM anyway)?

The ugly

To support plant__component__subcomponent => AlternativeComponent(), I used ScopedValues to thread those parameters. I believe that it's broadly fine, but there might be edge cases I haven't thought about.

TODO

If this PR looks appealing, then the remaining items would be

  • Clean up, comment
  • Conform to the style guide
  • Validate kwarg names (at the moment misspelled constructor kwargs are ignored)
  • Squash, or redo the commit history
  • Figure out a solution for the commented out tests (from add test for passing params as structural params #3995)
  • Test against ModelingToolkitStandardLibrary
  • More tests

Let me know what you think! If this is too disruptive, I can tackle another direction. Most of these features can be implemented without getting rid of the RHS parsing code, but it's a lot more work.

@cstjean
Copy link
Contributor Author

cstjean commented Nov 25, 2025

I may have some understanding of the current test failure. It seems that, with

@mtkmodel SimpleLag begin
    @structural_parameters begin
        K # Gain
    end
end

@mtkmodel DoubleLag begin
    @parameters begin
        K1, [description="Proportional gain 1"]
    end
    @components begin
        lag1 = SimpleLag(K = K1)
    end
end

When passing the parameter K1 to SimpleLag as a structural parameter, K1's metadata gains a SymScope => ParentScope(LocalScope()) entry. Is that correct? I'd appreciate a pointer to the logic that achieves that; it's not in model_parsing.jl AFAICT.

On this branch, SimpleLag's K is the same object as DoubleLag's K1, so it does not have a SymScope entry. This is perhaps why it errors with double_lag₊lag1₊K1 is present in the system but double_lag₊lag1₊K1 is not an unknown.

@cstjean
Copy link
Contributor Author

cstjean commented Nov 25, 2025

I figured it out (@named uses default_to_parentscope) and fixed it in 4086714

@AayushSabharwal
Copy link
Member

This is... interesting. Given that @mtkmodel is being deprecated in v11, I don't mind this as long as it passes tests. It might be worth holding off a bit, I'll try and get v11 out ASAP and this can be retargeted there. The parsing code hasn't changed in v11, we're just not maintaining the @mtkmodel syntax.

@ChrisRackauckas
Copy link
Member

We could spawn it off to a separate subpackage. I don't think anyone really plans to maintain it from the core SciML group but it could stay alive in that way, just we shouldn't use it in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replacing subsystems

3 participants