Skip to content

Fix @mtkmodel to work without using ModelingToolkit #3643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+27 −4
Open

Fix @mtkmodel to work without using ModelingToolkit #3643

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/systems/model_parsing.jl
View file
Open in desktop
Original file line number Diff line number Diff 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 Down Expand 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
22 changes: 22 additions & 0 deletions test/model_parsing.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,3 +1058,25 @@ end
@test isequal(constrs[2], -4 + ex.y ≲ 0)
@test ModelingToolkit.get_consolidate(ex)([1, 2]) ≈ 1 + log(2)
end

module NoUsingMTK # From #3640
using ModelingToolkit: @mtkmodel, @variables, @parameters, t_nounits as t

@mtkmodel MyModel begin
@variables begin
x(t)
y(t)
end
@parameters begin
a
end
@equations begin
y ~ a * x
end
end
end

@testset "Model can be defined even without `using MTK` (#3640)" begin
# Just test that it runs without error
@test NoUsingMTK.MyModel(; name = :foo) isa ModelingToolkit.AbstractSystem
end
Loading