Skip to content

Support in-place interpolation of symbolic idxs #988

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

hersle
Copy link

@hersle hersle commented Apr 12, 2025

An attempt at fixing SciML/OrdinaryDiffEq.jl#2562 and making sure the documented in-place interpolation API actually works consistently with MTK, too.

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Copy link
Member

@ChrisRackauckas ChrisRackauckas left a comment

Choose a reason for hiding this comment

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

This isn't the best solution since it allocates. But one step at a time, at least making this work should come first.

@ChrisRackauckas
Copy link
Member

I think the tests are showing this is leading some issues / ambiguities?

@hersle
Copy link
Author

hersle commented Apr 22, 2025

Yeah, I got started on this and left it hanging a little. Will see if I can get back on it in not too long.

@hersle hersle force-pushed the inplace_symbolic_idxs branch from 737942e to 778467a Compare April 22, 2025 19:01
@hersle
Copy link
Author

hersle commented Apr 23, 2025

Can you help me understand how the downstream testing works?

For example, IntegrationTest / OrdinaryDiffEq.jl/Interface/1 (pull_request) passes when I dev OrdinaryDiffEq SciMLBase locally (both at or ahead of the latest master branches):

julia> Pkg.status()
  ...
  [1dea7af3] OrdinaryDiffEq v6.95.0 `C:\Users\herma\.julia\dev\OrdinaryDiffEq`      
  [0bca4576] SciMLBase v2.85.0 `C:\Users\herma\.julia\dev\SciMLBase`
  ...

julia> include(raw"C:\Users\herma\.julia\dev\OrdinaryDiffEq\test\interface\inplace_interpolation.jl");
Test Summary:                      | Pass  Total  Time
ODESolution interpolation (normal) |    4      4  0.0s
  1D                               |    2      2  0.0s
  2D                               |    2      2  0.0s
Test Summary:                         | Pass  Total  Time
ODESolution interpolation (not dense) |    4      4  0.0s
  1D                                  |    2      2  0.0s
  2D                                  |    2      2  0.0s
Test Summary:                          | Pass  Total  Time
ODESolution interpolation (fixed-step) |    4      4  0.0s
  1D                                   |    2      2  0.0s
  2D                                   |    2      2  0.0s
Test Summary:                                  | Pass  Total  Time
ODESolution interpolation (CompositeAlgorithm) |    4      4  0.0s
  1D                                           |    2      2  0.0s
  2D                                           |    2      2  0.0s

But the Github run fails. I have not checked the other tests, though.

@ChrisRackauckas
Copy link
Member

It grabs this package https://github.com/SciML/SciMLBase.jl/actions/runs/14602553998/job/41007048826?pr=988#step:6:177 devs the downstream https://github.com/SciML/SciMLBase.jl/actions/runs/14602553998/job/41007048826?pr=988#step:6:626 and then runs. Rebase, I did a lot of releases with minor test fixes today so maybe if it's just rerun on the latest master things will be better? If that doesn't work, make sure you dev'd OrdinaryDiffEqCore? And if you check that, I can dive in and see if I can figure out what's going on.

@hersle hersle force-pushed the inplace_symbolic_idxs branch from 589bf05 to ea89483 Compare April 25, 2025 22:08
@hersle
Copy link
Author

hersle commented Apr 25, 2025

Okay, thank you. Does that mean it's equivalent to an environment where SciMLBase and OrdinaryDiffEq are both deved and on the tip of master? I just rebased onto the latest SciMLBase to be sure, and that test still passes locally on my computer. I don't think the precise state of OrdinaryDiffEq is relevant anyway, since I'm just running this test file.

Can you try to run tests once more, unless it sounds like I'm misunderstanding something? 😅

@hersle hersle force-pushed the inplace_symbolic_idxs branch from ea89483 to 2737f51 Compare April 26, 2025 10:13
@hersle
Copy link
Author

hersle commented Apr 26, 2025

I was missing an in-place dispatch for t::Number, so more tests should pass now...

I'm still confused about this MethodError vs BoundsError, though. It might not be a problem with versioning, but simply that my computer and Github give different results. It passes locally with MethodError, e.g.:

julia> sol_ODE(out_VF, tt; idxs = 1:1)
ERROR: MethodError: Cannot `convert` an object of type Vector{Float64} to an object of type Float64
The function `convert` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  convert(::Type{T}, ::Union{Static.StaticBool{N}, Static.StaticFloat64{N}, Static.StaticInt{N}} where N) where T<:Number
   @ Static C:\Users\herma\.julia\packages\Static\SeEGr\src\Static.jl:427
  convert(::Type{T}, ::MultivariatePolynomials.AbstractPolynomialLike) where T<:Number
   @ MultivariatePolynomials C:\Users\herma\.julia\packages\MultivariatePolynomials\iTfZE\src\conversion.jl:96
  convert(::Type{T}, ::CartesianIndex{1}) where T<:Number
   @ Base multidimensional.jl:136
  ...

Stacktrace:
  [1] setindex!(A::Memory{Float64}, x::Vector{Float64}, i1::Int64)
    @ Base .\genericmemory.jl:243
  [2] unsafe_copyto!(dest::Memory{Float64}, doffs::Int64, src::Memory{Vector{Float64}}, soffs::Int64, n::Int64)
    @ Base .\genericmemory.jl:153
  [3] unsafe_copyto!
    @ .\genericmemory.jl:133 [inlined]
  [4] _copyto_impl!
    @ .\array.jl:308 [inlined]
  [5] copyto!
    @ .\array.jl:294 [inlined]
  [6] copyto!
    @ .\array.jl:319 [inlined]
  [7] copyto!
    @ .\broadcast.jl:966 [inlined]
  [8] copyto!
    @ .\broadcast.jl:925 [inlined]
  [9] materialize!(::Base.Broadcast.DefaultArrayStyle{…}, dest::Vector{…}, bc::Base.Broadcast.Broadcasted{…})
    @ Base.Broadcast .\broadcast.jl:883
 [10] materialize!
    @ .\broadcast.jl:880 [inlined]
 [11] (::ODESolution{…})(v::Vector{…}, t::StepRangeLen{…}, ::Type{…}, idxs::UnitRange{…}, continuity::Symbol)
    @ SciMLBase C:\Users\herma\.julia\dev\SciMLBase\src\solutions\ode_solutions.jl:409
 [12] #_#538
    @ C:\Users\herma\.julia\dev\SciMLBase\src\solutions\ode_solutions.jl:229 [inlined]
 [13] AbstractODESolution
    @ C:\Users\herma\.julia\dev\SciMLBase\src\solutions\ode_solutions.jl:224 [inlined]
 [14] top-level scope
    @ REPL[106]:1
Some type information was truncated. Use `show(err)` to see complete types.

But the Github run somehow gives BoundsError, which also sounds reasonable, since the test is testing an incompatible output array on purpose. Is @test_throws Union{MethodError, BoundsError} OK, or should we get to the bottom of this?

@ChrisRackauckas
Copy link
Member

I'm still confused about this MethodError vs BoundsError, though. It might not be a problem with versioning, but simply that my computer and Github give different results. It passes locally with MethodError, e.g.:

That's because tests are run with --bounds-check=true, so the difference of force enabling bounds check is making that a bounds check error on the indexing of a scalar.

@ChrisRackauckas
Copy link
Member

Check a=1.0; @inbounds a[1] in your REPL with --bounds-check=false/true and you'll see the difference.

@ChrisRackauckas
Copy link
Member

Is @test_throws Union{MethodError, BoundsError} OK

yes

end

# Below are many internal dispatches for different combinations of arguments to the main API
# TODO: could use a clever rewrite, since a lot of reused code has accumulated
Copy link
Member

Choose a reason for hiding this comment

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

The issue is that the symbolic dispatch is kept open, since there's more than one issymbolic type. We should union that in the future, but it needs JuliaSymbolics/SymbolicUtils.jl#737

Comment on lines 390 to 391
v .= map(eachindex(t)) do ti
sol.interp(u, t[ti], nothing, deriv, p, continuity)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
v .= map(eachindex(t)) do ti
sol.interp(u, t[ti], nothing, deriv, p, continuity)
if eltype(v) <: Number
v[ti] = sol.interp(t[ti], nothing, deriv, p, continuity)
else
for ti in eachindex(t)
sol.interp(v[ti], t[ti], nothing, deriv, p, continuity)
end
end

…tches to one; test in-place symbolic interpolation with one time value
@hersle
Copy link
Author

hersle commented Apr 26, 2025

That's because tests are run with --bounds-check=true, so the difference of force enabling bounds check is making that a bounds check error on the indexing of a scalar.

Oh, well that explains it. Thanks.

Let's see how this goes...

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.

2 participants