Skip to content

Fix downstream test sources for OrdinaryDiffEq 7#594

Merged
ChrisRackauckas merged 3 commits into
SciML:masterfrom
ChrisRackauckas-Claude:fix-downstream-tests-odeq7
May 18, 2026
Merged

Fix downstream test sources for OrdinaryDiffEq 7#594
ChrisRackauckas merged 3 commits into
SciML:masterfrom
ChrisRackauckas-Claude:fix-downstream-tests-odeq7

Conversation

@ChrisRackauckas-Claude
Copy link
Copy Markdown
Contributor

Note

Please ignore this PR until reviewed by @ChrisRackauckas.

Summary

Follow-up to #593. That PR fixed the resolver, but the downstream test sources still don't run because the OrdinaryDiffEq 6→7 transition broke several call sites that the old resolver was masking. This PR makes the actual safetestsets pass.

Changes

test/downstream/odesolve.jl:

  • Rosenbrock23(autodiff = false)Rosenbrock23(autodiff = AutoFiniteDiff()). OrdinaryDiffEq 7 rejects Bool for that kwarg and asks for an ADType.
  • Add using OrdinaryDiffEqRosenbrock (Rodas5 is no longer re-exported from OrdinaryDiffEq).
  • Add using RecursiveArrayToolsShorthandConstructors (needed for the AP[...] syntax introduced in 334bd95; runtests.jl Pkg.develops the package but never using-s it).
  • SciMLBase.successful_retcode(sol) → bare successful_retcode(sol). successful_retcode is reachable through using OrdinaryDiffEq; SciMLBase as a bare name is not. Avoiding using SciMLBase keeps SciMLBase/ADTypes out of [deps] — adding them as direct deps would unresolvably conflict with RAT 4.3's transitive SciMLBase 3 requirement.

test/downstream/downstream_events.jl:

  • Add using RecursiveArrayToolsShorthandConstructors (same AP[...] reason).

test/downstream/Project.toml:

  • Add OrdinaryDiffEqRosenbrock = "1, 2" and RecursiveArrayToolsShorthandConstructors.

Local verification

After this branch:

Test Summary: | Pass  Broken  Total     Time
ODE Solve     |    4       1      5  1m34.7s
Test Summary: | Total  Time
Events        |     0  2.4s
Test Summary:          | Broken  Total  Time
Measurements and Units |      1      1  4.2s
Test Summary: | Pass  Total  Time
TrackerExt    |    1      1  0.5s

All four Downstream-group safetestsets that runtests.jl exercises run to completion. The Broken totals are pre-existing @test_broken markers, not new failures.

Out of scope

  • test/downstream/symbol_indexing.jl (SymbolicIndexingInterface group) still has many failures from MTK 11 / SII indexing API changes — its own investigation.
  • test/downstream/adjoints.jl remains commented out in runtests.jl per the pre-existing TODO.

Test plan

  • ODE Solve Tests safetestset passes locally (4 pass, 1 pre-existing broken)
  • Event Tests with ArrayPartition safetestset runs without error
  • Measurements and Units safetestset passes (1 pre-existing broken)
  • TrackerExt safetestset passes (1 pass)
  • CI Tests - Downstream - Julia 1/lts/pre

The compat bump in the previous commit gets `activate_downstream_env`
past `Pkg.develop`, but the downstream test sources still don't run
because the OrdinaryDiffEq 7 resolution was previously masking three
issues:

- `Rosenbrock23(autodiff = false)` is no longer supported; OrdinaryDiffEq 7
  rejects the `Bool` form and asks for an `ADType`. Use `AutoFiniteDiff()`
  (already reachable through `using OrdinaryDiffEq`).
- `Rodas5` is no longer re-exported from `OrdinaryDiffEq`; pull it in
  via `using OrdinaryDiffEqRosenbrock` and add the package as a dep.
- `AP[...]` shorthand requires `RecursiveArrayToolsShorthandConstructors`
  in scope; runtests.jl `Pkg.develop`s the package but never `using`-s it,
  so the downstream test files silently never had it.
- `SciMLBase.successful_retcode(...)` requires `SciMLBase` as a bare
  binding, which `using OrdinaryDiffEq` doesn't provide. Switch to the
  unqualified `successful_retcode` (also already exported through
  OrdinaryDiffEq), avoiding the need to add SciMLBase as a direct dep.

Verified locally that the ODE Solve / Events / Measurements and Units /
TrackerExt safetestsets all run to completion after the fix.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
…test

MTK 11 reorders ODESystem unknowns ([y, x] in this case, not [x, y]), so
the hardcoded propertynames in the test/downstream/symbol_indexing.jl
Tables interface check no longer match what `propertynames(row)` and
`Tables.columnnames(row)` return on the solution rows. The data columns
in `Array(sol_ts)'` already follow MTK's unknowns ordering, so deriving
the names list from `unknowns(lv)` keeps names and data in sync and is
robust to future MTK reorderings.

Restores 357/358 assertions in the `DiffEqArray Indexing Tests` testset
on Julia 1/lts/pre. The remaining 1-test error is in the array-symbolic
indexing test at line 103 (`sol[x .+ [y, 2y, 3y]] ≈ ...`) — passes
standalone, errors only inside `@safetestset`'s isolated module; this
looks like a separate Symbolics/SII module-isolation interaction that
needs its own investigation.

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

Update: pushed an additional fix for test/downstream/symbol_indexing.jl.

MTK 11 reorders unknowns(ODESystem(D(x), D(y))) to [y, x] (was [x, y] under MTK 8/9), which broke the hardcoded propertynames in the test_tables_interface call at line 80. Switched to deriving the names from unknowns(lv) so the test is order-agnostic.

After this commit, DiffEqArray Indexing Tests goes from 0/358 → 357/358 assertions passing on Julia 1.11 locally.

The remaining 1-test error is at line 103 (sol[x .+ [y, 2y, 3y]] ≈ vcat.(getindex.((sol,), [x[1] + y, x[2] + 2y, x[3] + 3y])...)) — exercises symbolic indexing with a Symbolics.Arr{Num, 1}. The test passes when run standalone but errors only inside @safetestset's isolated module (the stacktrace shows a BoundsError-style broadcast shape mismatch via SymbolicIndexingInterface.GetStateIndex). Looks like a separate Symbolics/SII module-isolation interaction that needs its own investigation — not gating the bulk of the downstream tests, and I didn't want to @test_broken it since that would only hide a real regression.

`sol[arr_sym, :]` should be equivalent to `sol[arr_sym]` — both ask for
the full timeseries of the symbolic indices. Currently the `:`-suffixed
form errors with a broadcast shape mismatch inside SII's `GetStateIndex`
when the underlying index resolves to a `Vector{Int}` (i.e. when the
symbol is array-valued like `x(t)[1:3]`):

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

(SII's `(gsi::GetStateIndex)(::Timeseries, prob, i)` broadcasts `gsi.idx`
against `state_values(prob, i)` without wrapping it in a 1-tuple, so a
length-N timeseries broadcast against a length-K idx vector errors. A
companion fix is being submitted upstream.)

Short-circuiting `A[sym, :]` to `A[sym]` here both restores the intended
semantics and avoids the broken SII codepath.

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

Update: pushed commit `39c3708` with a RAT-side workaround for the remaining `sol[x, :]` failure.

Root cause

The error was `DimensionMismatch` inside `SymbolicIndexingInterface.GetStateIndex(::Timeseries, prob, i)` at `SII/src/state_indexing.jl:43`:
```julia
getindex.(state_values(prob, i), gsi.idx) # missing 1-tuple wrap
```
When `gsi.idx` is a scalar Int this works (broadcasts length-1 against the timeseries), but when the index is array-symbolic (e.g. `x(t)[1:3]`), `gsi.idx` is a `Vector{Int}` and the broadcast tries to zip length-N timeseries against length-K idx vector → error. The companion no-`i` overload at line 38 already wraps correctly (`(gsi.idx,)`).

Fix split

  • Upstream proper fix: SciML/SymbolicIndexingInterface.jl#157 — mirror the 1-tuple wrap.
  • Here: route `A[sym, :]` through `A[sym]` at the RAT dispatch site, since the no-args symbolic getindex returns the same `Vector{Vector{Float64}}` and avoids the broken SII codepath. Once SII Remove unreachable code #157 lands and releases, this RAT short-circuit can be removed.

Verified

```
Test Summary: | Pass Total Time
DiffEqArray Indexing Tests | 358 358 13m09.4s
```

@ChrisRackauckas ChrisRackauckas marked this pull request as ready for review May 18, 2026 11:40
@ChrisRackauckas ChrisRackauckas merged commit 100d576 into SciML:master May 18, 2026
35 of 37 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