SciML / OrdinaryDiffEq.jl

High performance ordinary differential equation (ODE) and differential-algebraic equation (DAE) solvers, including neural ordinary differential equations (neural ODEs) and scientific machine learning (SciML)
https://diffeq.sciml.ai/latest/
Other
536 stars 205 forks source link

Avoid throwing error if type inference on u0/t fails #2157

Closed mchitre closed 6 months ago

mchitre commented 6 months ago

Problem

The current implementation checks that typeof(u0/t) == typeof(du) by type inference using Base.promote_op(). If they are unequal, it throws an error. The problem is that the type inference can fail, even if the actual types are equal. As the documentation of Base.promote_op() says:

WARNING Due to its fragility, use of promote_op should be avoided. It is preferable to base the container eltype on the type of the actual elements. Only in the absence of any elements (for an empty result container), it may be unavoidable to call promote_op.

The problem is not hypothetical, but actually occurs at times, but is hard to pin down. For example, with Julia 1.10.2, if I create a new environment with just MonteCarloMeasurements.jl:

julia> using MonteCarloMeasurements

julia> Base.promote_op(/, typeof([Particles(1000, Normal(10.0, 0.1))]), typeof(oneunit(1.0)))
Any

julia> typeof([Particles(1000, Normal(10.0, 0.1))] / oneunit(1.0))
Vector{Particles{Float64, 1000}} (alias for Array{Particles{Float64, 1000}, 1})

the inference fails, as seen above. The exact same code works with same version of Julia when the environment has some other packages installed (not loaded), although the exact packages needed have been elusive.

Solution

The solution is simply to accept that the inference may result in not the exact type, but a supertype sometimes.

Checklist

Additional context

Test could not be added because the conditions under which type inference fails are highly dependent on Julia version and also what other packages are installed and loaded in what order.

ChrisRackauckas commented 6 months ago

This looks good. If it's not possible to find a reasonable test today then I'd accept without a test.

mchitre commented 6 months ago

One discussion to have though, before we merge...

An alternative to skipping the check if type inference returns Any is to check if the type inference result is a supertype of typeof(f₀) instead. That will deal with type inference working, partially working or failing in a uniform way.

Something like: inferredtype >: typeof(f₀) or f₀ isa inferredtype

I don't know if partial inference failures are possible, or if there are downsides to doing this. So I kept the PR simpler, but if you feel this might be a better implementation, I am happy to amend.

ChrisRackauckas commented 6 months ago

I think the less inferred form can show up from some forms of array indexing, so I like that change.

mchitre commented 6 months ago

Changed to check f₀ isa inferredtype.

ChrisRackauckas commented 6 months ago

Codecov did something weird but tests look fine.