Skip to content

Fix iterate(::AbstractTimeseriesSolution) to respect AbstractArray contract#1322

Merged
ChrisRackauckas merged 3 commits intoSciML:masterfrom
ChrisRackauckas-Claude:fix-iterate-timeseriesolution-linear
Apr 23, 2026
Merged

Fix iterate(::AbstractTimeseriesSolution) to respect AbstractArray contract#1322
ChrisRackauckas merged 3 commits intoSciML:masterfrom
ChrisRackauckas-Claude:fix-iterate-timeseriesolution-linear

Conversation

@ChrisRackauckas-Claude
Copy link
Copy Markdown
Contributor

Summary

The existing Base.iterate(sol::AbstractTimeseriesSolution, state = 0) returns solution_new_tslocation(sol, state) — a fresh solution of the same concrete type with a bumped tslocation. That violates the AbstractArray contract (an array's first element must not have type identical to the container) and trips Julia 1.12's LinearAlgebra.norm_recursive_check:

ArgumentError: cannot evaluate norm recursively if the type of the initial
element is identical to that of the container

This surfaced in OrdinaryDiffEq's VectorOfArray/StructArray compatibility testsets (OrdinaryDiffEqLowStorageRK, OrdinaryDiffEqSSPRK) where sol_SA ≈ sol_SV compares two ODESolutions whose u is Vector{VectorOfArray{Float64, 2, StructVector{SVector{1, Float64}}}}. The generic Base.isapprox for AbstractArray falls through to LinearAlgebra.norm, which on 1.12 hits the check.

Change

Replace the custom iterate with the natural linear-indexing form:

function Base.iterate(sol::AbstractTimeseriesSolution, state = 1)
    state > length(sol) && return nothing
    return (@inbounds sol[state], state + 1)
end

For VectorOfArray-backed solutions this yields scalars (matching eltype(sol)), which is what LinearAlgebra.norm and isapprox expect. solution_new_tslocation is still exported for the plot recipe to use directly — it was only ever load-bearing inside this method.

Breaking behavior note

Minor behavior change: downstream code that did for s in sol expecting s to be a full solution-like object (with .retcode, .prob, etc.) will need to change. In practice such callers should iterate sol.u / sol.t explicitly, or pair solution_new_tslocation with eachindex(sol.u). One such site lives in OrdinaryDiffEq.jl/test/ad/ad_tests.jl:388 (for s in sol with s.retcode); that's a test-only call and can be fixed in-place to read sol.retcode once.

Bumped to 3.4.0 to flag the iteration semantics change.

Test plan

  • Added @testset "iterate does not yield the container" in test/solution_interface.jl with: type check, non-equality check, isfinite(norm(sol)), sol ≈ sol. All pass on Julia 1.12.
  • Unblocks the upstream failing VectorOfArray/StructArray compatibility testsets in OrdinaryDiffEqLowStorageRK and OrdinaryDiffEqSSPRK (verified locally against monorepo-dev'd sublibs).
  • CI on this PR
  • Run OrdinaryDiffEq.jl#3513 against this to confirm the downstream fix can be closed.

Closes-replaces-workaround-for: SciML/OrdinaryDiffEq.jl#3513

🤖 Generated with Claude Code

…contract

The previous implementation returned `solution_new_tslocation(sol, state)` —
a fresh solution of the same concrete type, with successively larger
`tslocation`. That violates the AbstractArray contract (element type must not
equal container type) and causes `LinearAlgebra.norm(sol)` on Julia 1.12 to
trip `norm_recursive_check`:

    ArgumentError: cannot evaluate norm recursively if the type of the
    initial element is identical to that of the container

This surfaced as failures in OrdinaryDiffEq's `VectorOfArray/StructArray
compatibility` testsets (LowStorageRK, SSPRK) where `sol_SA ≈ sol_SV`
compares two `ODESolution`s whose `u` is `Vector{VectorOfArray{Float64, 2,
StructVector{SVector{1, Float64}}}}`. The generic `Base.isapprox` for
`AbstractArray` falls through to `norm`, which hit the check.

Replace the custom `iterate` with the natural linear-indexing form
(`sol[state]`), which matches the AbstractArray iteration contract and
yields scalars for `VectorOfArray`-backed solutions. `solution_new_tslocation`
remains exported for callers (plot recipe) that want the `tslocation`-
stepping semantics directly; it was only ever load-bearing inside this
iterate method.

Behaviorally, this is a minor breaking change for any downstream code that
did `for s in sol` expecting `s` to be a full solution object (previously the
same container with a bumped `tslocation`). Such callers should either
iterate `sol.u` / `sol.t` explicitly, or index with `solution_new_tslocation`.

Bump to 3.4.0 to reflect the iteration semantics change.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Per review: the cleanest fix is to not define `iterate` on
`AbstractTimeseriesSolution` at all. The AbstractArray fallback (iterate
over `eachindex(sol)`) is the only iteration that is type-consistent with
`eltype(sol)` and `size(sol)` the solution already declares. Any custom
iterate just risks re-introducing the same class of bug.

No behavior change from the previous commit in practice (both forms
delegated to `sol[state]`); this just removes the now-redundant wrapper.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas-Claude
Copy link
Copy Markdown
Contributor Author

Updated per review: deleted the custom iterate entirely rather than replacing it with an explicit linear-indexing form. The AbstractArray fallback (iterate over eachindex(sol)) is the only iteration that's type-consistent with eltype(sol) / size(sol) — any custom implementation risks re-introducing the same class of bug. Same behavior as the previous commit in practice (both delegated to sol[state]), just less code.

Comment thread src/solutions/solution_interface.jl Outdated
Comment thread test/solution_interface.jl Outdated
Co-authored-by: Christopher Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas ChrisRackauckas merged commit ca95e2e into SciML:master Apr 23, 2026
39 of 47 checks passed
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