+27 −4
Open

Fix @mtkmodel to work without using ModelingToolkit #3643

Changes from 1 commit
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4ba071
Fix @mtkmodel to work without `using ModelingToolkit`
cstjeanMay 20, 2025
23db74e
Write test for #3640
cstjeanMay 20, 2025
52de720
Respect 92 char line length limit
cstjeanMay 20, 2025
321836b
Run JuliaFormatter
cstjeanMay 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Failed to load files.
Next Next commit
Fix @mtkmodel to work without using ModelingToolkit
See #3640

Feels like `const MTK = ModelingToolkit` should be at the top-level for
convenience.
  • Loading branch information
@cstjean
cstjean committedMay 20, 2025
commit a4ba071e2a93583ed7984a849914a4fcdba2d1ff
Original file line numberDiff line numberDiff line change
Expand Up@@ -42,8 +42,9 @@ function flatten_equations(eqs::Vector{Union{Equation, Vector{Equation}}})
end

function _model_macro(mod, fullname::Union{Expr, Symbol}, expr, isconnector)
MTK = ModelingToolkit
if fullname isa Symbol
name, type = fullname, :System
name, type = fullname, :($MTK.System)
else
if fullname.head == :(::)
name, type = fullname.args
Expand DownExpand Up@@ -74,9 +75,9 @@ function _model_macro(mod, fullname::Union{Expr, Symbol}, expr, isconnector)
push!(exprs.args, :(parameters = []))
# We build `System` by default; vectors can't be created for `System` as it is
# a function.
push!(exprs.args, :(systems = ModelingToolkit.AbstractSystem[]))
push!(exprs.args, :(equations = Union{Equation, Vector{Equation}}[]))
push!(exprs.args, :(defaults = Dict{Num, Union{Number, Symbol, Function}}()))
push!(exprs.args, :(systems = $MTK.AbstractSystem[]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is prepending $MTK. to everything really the solution? What if we did this instead:

Suggested change
push!(exprs.args, :(systems = $MTK.AbstractSystem[]))
push!(exprs.args, :(systems = $AbstractSystem[]))

Also wouldn't this need to be done in more places? e.g. where @variables is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the discussion around https://discourse.julialang.org/t/adding-method-to-function-in-macro/128613/6 . If you're going to esc the expression, then you need either $MTK.AbstractSystem or $AbstractSystem, but there are places where the latter will fail. In particular, you can't $@some_macro obviously. I always use the former style for that reason, but I don't mind changing this PR to $AbstractSystem if that's your preference.

Also wouldn't this need to be done in more places?

Yes, absolutely. I didn't want to venture outside of my MTK comfort zone.

e.g. where @variables is called.

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the discussion around https://discourse.julialang.org/t/adding-method-to-function-in-macro/128613/6 . If you're going to esc the expression, then you need either $MTK.AbstractSystem or $AbstractSystem, but there are places where the latter will fail

I see, thanks for clarifying. $MTK is fine, then.

e.g. where @variables is called.

?

While parsing @variables inside @mtkmodel, we generate code that calls Symbolics.@variables and MTK.@parameters. I assume those need to be qualified as well?

Copy link
Contributor Author

@cstjean cstjean May 20, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I always prefix functions, types and macros with $MyModule. when using esc. It's the flip side of skipping macro hygiene. It can also prevent unnecessary conflicts if ModelingToolkit and SomeOtherLibrary export the same symbol.

In this particular case it doesn't matter because the user will feel compelled to import ModelingToolkit: @variables anyway, but that's not reliable in general.

push!(exprs.args, :(equations = Union{$MTK.Equation, Vector{$MTK.Equation}}[]))
push!(exprs.args, :(defaults = Dict{$MTK.Num, Union{Number, Symbol, Function}}()))

Base.remove_linenums!(expr)
for arg in expr.args
Expand Down