default for array variable broken

[baggepinnen]

The following definition used to be fine, but is now broken on latest master

ERROR: LoadError: KeyError: key :parameters not found
Stacktrace:
 [1] getindex
   @ ./dict.jl:498 [inlined]
 [2] parse_variable_def!(dict::Dict{…}, mod::Module, arg::Expr, varclass::Symbol, kwargs::OrderedCollections.OrderedSet{…}, where_types::Vector{…}; def::Nothing, indices::Nothing, type::Type, meta::Dict{…})
   @ ModelingToolkit ~/.julia/dev/ModelingToolkit/src/systems/model_parsing.jl:329
 [3] parse_variable_def!

The issue is the default value for r, if I remove it things work.

@mtkmodel Fixed begin
    @parameters begin
        (r[1:2] = [0, 0]), [description = "Fixed absolute xy-position, resolved in planarWorld frame"]
        phi = 0, [description = "Fixed angle"]
    end

    @components begin
        frame_b = Frame()
    end

    @equations begin
        frame_b.x ~ r[1]
        frame_b.y ~ r[2]
        frame_b.phi ~ phi
    end
end

the error is

[ven-k]

Actually, parsing has gotten better; () aren't needed anymore.

This works:

@mtkmodel Fixed begin
    @parameters begin
        r[1:2] = [0, 0], [description = "Fixed absolute xy-position, resolved in planarWorld frame"]
        phi = 0, [description = "Fixed angle"]
    end
   ...

[baggepinnen]

oki that's good, but breaking what was previously working (and required!) is not good

[ChrisRackauckas]

The () was actually incorrect... it's kind of surprising that parsing actually worked 😅 . It was definitely not intended and not documented.

[baggepinnen]

It used to error without it but not with it

[baggepinnen]

Also, yingbo closed the issue complaining about this problem by showing to use parenthesis instead
#2298 (comment)
I thus consider this breaking

[ChrisRackauckas]

Can you show where it's documented?

[baggepinnen]

I can not, but are you saying that following the advice of a core developer should lead to your code breaking in a patch release? Especially when the issue complaining about this exact problem was closed with this very advice? We must stop breaking people's code like this if we want people to look at MTK as a reliable tool.

[ChrisRackauckas]

The 2024-2025 push is to make MTK match the rest of the reliability in SciML by strictly defining its interfaces, documentation, and better erroring on the breaks. A core developer shared an undocumented, untested, and unpromoted script that was a hacky temporary workaround. This is the exact opposite of the kind of code you're asking for and is exactly the kind of thing that anyone who wants reliability will crack down. Yes, prior unchecked interfaces in this package had traditionally let things slip through. When we turn on checks of interfaces and make things match how they are documented to work, you will get errors if you break the interface. I suggest not relying on hacks around the documented interface, helping with the documentation clarity of the interfaces, and erroring on things to better define the boundary. Hopefully there aren't many left as we have chopped out most of them with the parsing changes and the move to a documented SymbolicIndexingInterface.

[isaacsas]

@ChrisRackauckas it would be nice as part of this push to also have an API for people developing on top of MTK who may need access to some of the internals (i.e. to declare some of the currently internal symbolic handling code, system code, and macro parsing code public).

[isaacsas]

(That is another source of things breaking in non-breaking MTK releases we see quite frequently in Catalyst, which is our fault for using such functions, but really the only option in many cases short of copying the code from MTK into Catalyst.)

[ChrisRackauckas]

Most of the internal symbolic handling code was made public API when Symbolics.jl was section out. If there's anything missing from that it should get an issue. At this point I think almost all of it is documented Symbolics.jl or SymbolicUtils.jl actions?

For the system code, yeah there's still a few things we need to document and make API. Some of the query functions in particular are nice to use and likely should get tested, like some of the stuff around metadata.

Macro parsing code 😅 , I'm not sure about how public API that should really be... unless it's sectioned off to some macro parsing utils package or something. That's quite internal stuff.

[YingboMa]

Why shouldn't

        (r[1:2] = [0, 0]), [description = "Fixed absolute xy-position, resolved in planarWorld frame"]

be supported? Also, by Julia syntax,

begin
       r = [0, 0], [description = "Fixed absolute xy-position, resolved in planarWorld frame"]
       a
end

means

begin
       r = ([0, 0], [description = "Fixed absolute xy-position, resolved in planarWorld frame"])
       a
end

so

        (r[1:2] = [0, 0]), [description = "Fixed absolute xy-position, resolved in planarWorld frame"]

is always the correct syntax anyway.

Proof:

julia> ex = :(begin
       r = [0, 0], [description = "Fixed absolute xy-position, resolved in planarWorld frame"]
       a
       end)
quote
    #= REPL[14]:2 =#
    r = ([0, 0], [description = "Fixed absolute xy-position, resolved in planarWorld frame"])
    #= REPL[14]:3 =#
    a
end

[YingboMa]

The revert PR #3092 fixes this issue.

[ven-k]

Note that, the

r[1:2] = [0, 0], [description = "Fixed absolute xy-position, resolved in planarWorld frame"]

syntax was picked for mtkmodel and internally parser handled was set to correctly assign default and metadata values (and we had unit tests for this). This was to avoid the () that would be necessary with vanilla-@variables.

That's why (r[1:2] = [0, 0]), [...] wasn't part of the test suite and when updated this error was not caught.
However, there was nothing (code or docs) that explicitly disallowed that method either, resulting in confusion.

Now, with: #3107

The updated parser works with both syntaxes and has dedicated tests for both
Also, I've added a dedicated section in docs describing all the different ways to define arrays in mtkmodel.

[isaacsas]

If this is going to be added to the DSL why not add to the standalone macros too? Seems likely to be confusing to people otherwise.