Skip to content

Conversation

baggepinnen
Copy link
Contributor

@baggepinnen baggepinnen commented Sep 25, 2025

attempt to close #2813

This only handles defaults that are a single Num, like r=r_def, it does not yet handle r_def being an expression with multiple variables

Copy link
Contributor

github-actions bot commented Sep 25, 2025

Benchmark Results (Julia vlts)

Time benchmarks
master 42688d5... master / 42688d5...
ODEProblem 0.0858 ± 0.0071 s 0.0833 ± 0.0068 s 1.03 ± 0.12
init 0.0598 ± 0.0024 ms 0.0587 ± 0.0016 ms 1.02 ± 0.05
large_parameter_init/ODEProblem 1.11 ± 0.0016 s 1.11 ± 0.0037 s 0.999 ± 0.0037
large_parameter_init/init 0.111 ± 0.0044 ms 0.109 ± 0.0028 ms 1.01 ± 0.048
mtkcompile 0.0367 ± 0.0033 s 0.0378 ± 0.0016 s 0.971 ± 0.096
time_to_load 4.99 ± 0.026 s 5.08 ± 0.018 s 0.982 ± 0.0062
Memory benchmarks
master 42688d5... master / 42688d5...
ODEProblem 0.587 M allocs: 19.9 MB 0.587 M allocs: 19.9 MB 1
init 0.892 k allocs: 0.0463 MB 0.892 k allocs: 0.0463 MB 1
large_parameter_init/ODEProblem 4.32 M allocs: 0.168 GB 4.3 M allocs: 0.166 GB 1.01
large_parameter_init/init 1.74 k allocs: 0.0858 MB 1.74 k allocs: 0.0858 MB 1
mtkcompile 0.237 M allocs: 8.75 MB 0.243 M allocs: 9.02 MB 0.971
time_to_load 0.153 k allocs: 14.5 kB 0.153 k allocs: 14.5 kB 1

Copy link
Contributor

github-actions bot commented Sep 25, 2025

Benchmark Results (Julia v1)

Time benchmarks
master 42688d5... master / 42688d5...
ODEProblem 0.0793 ± 0.0041 s 0.0765 ± 0.0055 s 1.04 ± 0.092
init 0.0526 ± 0.012 ms 0.0524 ± 0.011 ms 1 ± 0.31
large_parameter_init/ODEProblem 1.09 ± 0.015 s 1.06 ± 0.021 s 1.03 ± 0.024
large_parameter_init/init 0.094 ± 0.0091 ms 0.0939 ± 0.01 ms 1 ± 0.15
mtkcompile 0.0344 ± 0.00098 s 0.0343 ± 0.00062 s 1 ± 0.034
time_to_load 4.92 ± 0.061 s 4.9 ± 0.044 s 1.01 ± 0.015
Memory benchmarks
master 42688d5... master / 42688d5...
ODEProblem 0.577 M allocs: 18.8 MB 0.578 M allocs: 18.8 MB 1
init 0.799 k allocs: 30.9 kB 0.799 k allocs: 30.9 kB 1
large_parameter_init/ODEProblem 4.39 M allocs: 0.16 GB 4.4 M allocs: 0.159 GB 1
large_parameter_init/init 1.65 k allocs: 0.0696 MB 1.65 k allocs: 0.0696 MB 1
mtkcompile 0.236 M allocs: 8.17 MB 0.242 M allocs: 8.38 MB 0.975
time_to_load 0.159 k allocs: 11.2 kB 0.159 k allocs: 11.2 kB 1

for var in expr_vars
var_unwrapped = unwrap(var)
# If any variable in the expression is from parent scope, don't namespace
if isparameter(var_unwrapped) && !(var_unwrapped in sys_params || var_unwrapped in sys_unknowns)
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
if isparameter(var_unwrapped) && !(var_unwrapped in sys_params || var_unwrapped in sys_unknowns)
if isparameter(var_unwrapped) &&
!(var_unwrapped in sys_params || var_unwrapped in sys_unknowns)

Comment on lines +1170 to +1171
Dict((isparameter(k) ? parameters(sys, k) : unknowns(sys, k)) =>
(should_namespace(v) ? namespace_expr(v, sys) : v)
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
Dict((isparameter(k) ? parameters(sys, k) : unknowns(sys, k)) =>
(should_namespace(v) ? namespace_expr(v, sys) : v)
Dict((isparameter(k) ? parameters(sys, k) :
unknowns(sys, k)) => (should_namespace(v) ? namespace_expr(v, sys) : v)

Copy link
Member

@AayushSabharwal AayushSabharwal left a comment

Choose a reason for hiding this comment

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

Tests fail

@baggepinnen
Copy link
Contributor Author

I don't think I have sufficient insight into the internals to make everything pass. This was enough to allow me to move forward on a time critical problem, so I'll probably leave it here for now

@AayushSabharwal
Copy link
Member

Makes sense, I'll try and take a look when I can.

@baggepinnen
Copy link
Contributor Author

Here's another test case that fails both on latest release and this PR branch, wrong number of defaults generated

using ModelingToolkit
using ModelingToolkit: t_nounits as t, D_nounits as D

@mtkmodel inner2 begin
    @parameters begin
        a[1:2] = a
    end
    @variables begin
        x(t)[1:2] = zeros(2)
    end
    @equations begin
        D(x) ~ a
    end
end

@mtkmodel outer2 begin
    @parameters begin
        a1 = 1
        a2 = 1
    end
    @components begin
        i = inner2(a=[a1,a2])
    end
    @variables begin
     y(t)[1:2] = zeros(2)
     dy(t)[1:2]
    end
    @equations begin
        D(y) ~ dy
        dy ~ [a1, a2]
    end
end

@named o = outer2()
o = complete(o)
ssys = mtkcompile(o)
varmap = []

prob = ODEProblem(ssys, varmap, (0, 1))

This looks fishy

julia> MTK=ModelingToolkit;filter(kvp -> !MTK.isinitial(kvp[1]) && !MTK.isparameter(kvp[1]), defaults(ssys))
Dict{Any, Any} with 6 entries:
  (i₊x(t))[2] => 0.0
  (i₊x(t))[1] => 0.0
  (y(t))[2]   => 0.0
  (y(t))[1]   => 0.0
  (dy(t))[2]  => NoValue()
  (dy(t))[1]  => NoValue()

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.

parameter default given by other parameter leads to incorrect codegen
2 participants