Skip to content

GetStateIndex: wrap idx so it broadcasts as scalar across timeseries#157

Merged
ChrisRackauckas merged 2 commits into
SciML:masterfrom
ChrisRackauckas-Claude:fix-getstateindex-vector-colon
May 18, 2026
Merged

GetStateIndex: wrap idx so it broadcasts as scalar across timeseries#157
ChrisRackauckas merged 2 commits into
SciML:masterfrom
ChrisRackauckas-Claude:fix-getstateindex-vector-colon

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Contributor

Note

Please ignore this PR until reviewed by @ChrisRackauckas.

Summary

(gsi::GetStateIndex)(::Timeseries, prob, i) (at `src/state_indexing.jl:43`) currently does:

```julia
getindex.(state_values(prob, i), gsi.idx)
```

This works when `gsi.idx` is a scalar Int (broadcasts as length-1 against the time axis), but errors when `gsi.idx` is array-valued — e.g. a `Vector{Int}` produced when the symbolic is array-typed like `x(t)[1:3]` — because the broadcast then tries to zip the length-N timeseries against the length-K index vector:

```
DimensionMismatch: arrays could not be broadcast to a common size:
a has axes Base.OneTo(N) and b has axes Base.OneTo(K)
```

The companion no-`i` overload (line 38) already handles this with a 1-tuple wrap:

```julia
function (gsi::GetStateIndex)(::Timeseries, prob)
return getindex.(state_values(prob), (gsi.idx,)) # wrapped
end
```

This PR mirrors that wrap on the with-`i` overload so array-valued `idx` is always broadcast as a scalar across the time axis.

Reproduction

Triggered by `sol[x(t)[1:3], :]` in the RecursiveArrayTools.jl downstream tests with MTK 11 + OrdinaryDiffEq 7. After fix, the call returns the expected `Vector{Vector{Float64}}` matching `sol[x(t)[1:3]]`.

Side note

The same issue is being worked around on the RecursiveArrayTools.jl side in SciML/RecursiveArrayTools.jl#594 by short-circuiting `A[sym, :] → A[sym]` for symbolic indices; once this SII fix lands and gets released, that workaround can be removed.

Test plan

  • Existing `Pkg.test()` passes locally (modulo the pre-existing `jet_test.jl:161` `remake_buffer` failure that's unrelated to this change and reproduces on master).
  • RAT downstream `DiffEqArray Indexing Tests` safetestset goes 357/358 → 358/358 when this fix is loaded.
  • CI

`(gsi::GetStateIndex)(::Timeseries, prob, i)` previously did

    getindex.(state_values(prob, i), gsi.idx)

which works when `gsi.idx` is scalar (broadcasts as length-1 against the
time axis), but errors when `gsi.idx` is array-valued — e.g. a
`Vector{Int}` produced when the user requested an array-symbolic like
`x(t)[1:3]` — because then the broadcast tries to zip the length-N
timeseries against the length-K index vector and errors:

    DimensionMismatch: arrays could not be broadcast to a common size:
    a has axes Base.OneTo(N) and b has axes Base.OneTo(K)

The no-`i` overload at line 38 already handles this correctly by
wrapping `gsi.idx` in a 1-tuple. Mirror that here so an array-valued
`idx` is always broadcast as a scalar across the time axis.

Reproduces in RecursiveArrayTools.jl downstream tests with MTK 11 +
OrdinaryDiffEq 7:

    sol[x(t)[1:3], :]
    # ERROR: DimensionMismatch (10 vs 3)

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
The previous tests for `getsym(sys, [:x, :y])(sol, ::Colon)` only exercise
the `MultipleGetters` indexer path — `getsym` builds one `GetStateIndex`
per symbol, never a single `GetStateIndex{Vector{Int}}`. The buggy
codepath fires when downstream packages (e.g. ModelingToolkit) bind an
array-valued symbolic like `x(t)[1:3]` to a single contiguous set of
state indices and construct `GetStateIndex(Vector{Int})` directly. This
test constructs that shape manually and exercises each non-`Int`/-`CartesianIndex`
`i` overload (`:`, `1:N`, `eachindex`, `BitVector`) so a future
regression would be caught here.

Reproduces a `DimensionMismatch: arrays could not be broadcast to a
common size: a has axes Base.OneTo(4) and b has axes Base.OneTo(2)`
without the `(gsi.idx,)` wrap.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas ChrisRackauckas marked this pull request as ready for review May 18, 2026 13:08
@ChrisRackauckas ChrisRackauckas merged commit 59a1191 into SciML:master May 18, 2026
10 of 14 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