Skip to content
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.

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

Indexing solution with symbolic array is slow #2112

Closed
MarcBerliner opened this issue Mar 7, 2023 · 2 comments
Closed

Indexing solution with symbolic array is slow #2112

MarcBerliner opened this issue Mar 7, 2023 · 2 comments
Labels

Comments

@MarcBerliner
Copy link
Contributor

using ModelingToolkit, OrdinaryDiffEq
@variables t u(t)[1:10] = 1
u = collect(u)

eqs = Differential(t).(u) .~ (1:length(u))

@named sys = ODESystem(eqs,t,u,[]; tspan=(0, 10))

prob = ODEProblem(sys)

sol = solve(prob, Tsit5(), tstops=range(prob.tspan...; length=10000))

using BenchmarkTools
@btime getindex($sol, $u);
# 1.649 s (12584906 allocations: 1.47 GiB)

Seems to scale roughly linearly with the length of the solution

sol = solve(prob, Tsit5(), tstops=range(prob.tspan...; length=100))
@btime getindex($sol, $u);
# 15.070 ms (124909 allocations: 15.01 MiB)

Indexing the solution with 10 separate scalar symbols is reasonably fast. Not exactly a 1:1 comparison, but this should give a reasonable order of magnitude for the expected speed.

sol = solve(prob, Tsit5(), tstops=range(prob.tspan...; length=10000))
@btime getindex.($(Ref(sol)), $u);
# 312.833 μs (1259 allocations: 932.17 KiB)
@xtalax
Copy link
Member

xtalax commented Mar 7, 2023

I think this is because of sym_to_index, which is a search through the states of the system, if there is no ordering here this would have to be an O(n) operation.

I've noticed that unsurprisingly a hashmap (Dict) indexed with symbols is faster for even a low number of states, though if you try to index with more complex expressions this becomes a trade off at low n as the hashing cost increases, we should probably be creating index maps to levarage this.

I add a sym_map that can do this in #2071, once this is merged we can move to using this by default.

@ChrisRackauckas
Copy link
Member

Fixed with new SII tools, or repeat of the SII issue #2338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants