[ChrisRackauckas-Claude]

Summary

get_all_timeseries_indexes(sc::SymbolCache, sym::AbstractArray) previously used mapreduce(..., union, ...), allocating a new Set per element and another per union step. This replaces that with a single accumulator Set, pushing scalar results directly and recursing for Expr/array elements. Correctness (including Set{Any} element type) is unchanged for plain, timeseries-parameter, and mixed [:x, :b, :(x + a)] cases.

Performance (a CI box, Julia 1.12.7, JULIA_NUM_THREADS=1 taskset -c 17, @benchmark minima)

Call	Before	After
get_all_timeseries_indexes N=1000	382.0 µs / 15000 allocs	24.0 µs / 4 allocs
getu(sys, vars) N=1000	413.9 µs	55.6 µs

Allocation regression test (@allocations via function barrier, N=100): fails on unfixed code with 1500 <= 16, passes with the fix (<= 16). Verified on Julia 1.10.12 and 1.12.7.

Test group tails

GROUP=Core: all passed, including Core/symbol_cache_test.jl | 88 88 and Core/parameter_timeseries_collection_test.jl | 227 227. Final: Testing SymbolicIndexingInterface tests passed.

GROUP=QA: QA/jet_test.jl | 45 45, QA/qa.jl | 22 22. Final: Testing SymbolicIndexingInterface tests passed.

Not verified

• Downstream packages (ModelingToolkit, DiffEq, etc.)
• GROUP=Downstream / 32-bit Core lane
• AirspeedVelocity CI benchmarks
• Expr-only get_all_timeseries_indexes path (still uses mapreduce)

Reviewer pushback

• Whether <= 16 is the right allocation bound (measured fixed cost is 4 allocs / 320 B for the result Set{Any}).
• Whether the Expr path should get the same accumulate treatment in a follow-up.
• Whether _accumulate_timeseries_indexes! should stay unexported (current choice; no version bump).

Please ignore until reviewed by @ChrisRackauckas.

Risk assessment

Independent review: pending

🤖 Generated with Cursor Agent 2026.10.01-14929f9 (model: unknown (Cursor auto)), transcript /sii-tsidx/log.txt; orchestrated by Claude Code (claude-opus-5-5[1m]) https://claude.ai/code/session_01LPHREnnonfLg1VcE1EJovv

Made with Cursor

Independent review: Devin Fusion (fusion-claude-opus-5-5-high-sidekick-swe-2-medium) rated it low, verdict MERGE: #191 (comment)

Review findings addressed

1. Custom scalar get_all_timeseries_indexes ignored inside arrays (blocking): fixed in fedb68a. The array method now unions the public scalar get_all_timeseries_indexes(sc, el) result for every element that is not a Symbol, and that includes Expr, nested arrays and downstream types. Symbol elements keep the allocation-free path, since a downstream package cannot add a method for (SymbolCache, Symbol) without type piracy. The scalar method and the Symbol fast path share the private _timeseries_index helper, which also removes the duplicated classification logic noted in the earlier review.
   • New tests in test/symbol_cache_test.jl: a downstream-owned WrappedTimeseriesExpression with its own scalar method, checked inside [wrapped] and mixed with [:b, wrapped]; and a check that [s] == s and [[s]] == s for every kind of symbol.
   • Before (PR head 2e5064e): sc | 107 2 109. The failures were Set{Any}() == Set(Any[2, ContinuousTimeseries()]) and Set(Any[1]) == Set(Any[2, ContinuousTimeseries(), 1]).
   • After: sc | 109 109.
   • Allocation bound: <= 16 still passes. At N=1000 the call still makes 4 allocations and takes about 24 µs, the same as 2e5064e.
   • Verification: GROUP=Core passes on Julia 1.12.7 and 1.10.12 (Core/symbol_cache_test.jl | 109 109). GROUP=QA passes on 1.12.7 (jet 45/45, qa 22/22) and 1.10.12. Runic and typos are clean.
   • Not run: Julia 1.13.0. On 1.13, Pkg.test cannot resolve JET in this environment, so neither Core nor QA got as far as running a test. Downstream was not run either.

---

🤖 Posted by an AI agent — harness: Claude Code 2.1.301 · model: claude-opus-5-5
Conversation: local session ID c40bb3ad-1a2d-4f2b-9aa7-6a1ad32667de (headless, no URL)

Review findings addressed (round 2)

1. nothing timeseries index treated as absent: fixed in 522f0dd. The private helper _timeseries_index now returns Some(idx) when it finds an index and nothing only when the symbol is not a timeseries symbol. As a result, ParameterTimeseriesIndex(nothing, 1) gives Set([nothing]) again, for scalar and array queries alike.

   • New tests in test/symbol_cache_test.jl: the scalar query :p, the arrays [:p] and [:x, :p, :q], and a check that a non-timeseries :q still returns an empty set.

   • Results (Julia 1.12.7):

     Source	Result
     Before (fedb68a)	sc | 111 3 114, for example Set{Any}() == Set([nothing])
     After (522f0dd)	sc | 114 114
     Default branch (e9646b8) with the new test file	113/114; the only failure is the allocation bound 1500 <= 16, so the nothing-index tests pass there too

   • Unchanged: scalar return types (Set{ContinuousTimeseries}, Set{Int64}, Set{Any}, Set{Nothing}), and the cost at N=1000 (4 allocations, about 24 µs).

   • Verification: GROUP=Core passes on 1.12.7 and 1.10.12 (Core/symbol_cache_test.jl | 114 114). GROUP=QA passes on 1.12.7 (jet 45/45, qa 22/22) and 1.10.12 (jet 45/45, qa 20/20). Runic and typos are clean.

   • Not a breaking change: public behavior matches the default branch.

---

🤖 Posted by an AI agent — harness: Claude Code 2.1.301 · model: claude-opus-5-5
Conversation: local session ID c40bb3ad-1a2d-4f2b-9aa7-6a1ad32667de (headless, no URL)

Risk assessment (independent review, codex-cli gpt-6.1-sol, reviewed head 522f0dd)

• Risk: low
• Blast radius: SymbolCache scalar and array timeseries-index queries, including downstream getter construction. The optimization specializes Symbol elements; other elements retain public scalar dispatch. No new public names, dependency/license changes, test weakening, or observed breaking API changes. A version bump is not needed for this behavior-preserving optimization. I independently assessed risk before reading the author's risk claims.
• Evidence: Reviewed the whole PR at 522f0dd370583c16577341f4ada11e270f042d1d; rechecked the remote head at completion and it was unchanged. With JULIA_NUM_THREADS=4, the full Julia 1.10.12 Core group passed: 2,579 assertions across 14 files, including Core/symbol_cache_test.jl | 114 114; final output: Testing SymbolicIndexingInterface tests passed. The focused test file also passed 114/114 on Julia 1.13.0. Current CI has 12 pending checks and no completed failures, including Tests, IntegrationTest, Documentation, and format-check. No CI regression is currently observable; pending checks are not passing evidence.
• Independent review: codex-cli 0.163.0 gpt-6.1-sol rated it low, high confidence: both earlier regressions reproduce on their unfixed revisions and pass at this head, while the allocation regression test remains effective.
• Merge: needs human review — current CI is pending, so this is not yet eligible for automatic merging under the green-CI requirement. No code changes are requested.

---

🤖 Posted by an AI agent — harness: codex-cli · model: gpt-6.1-sol (independent reviewer; appended by Claude Code 2.1.301 · claude-opus-5-5)
Conversation: https://claude.ai/code/session_01VZjwf7Ux9YtTzwHGHMqnsq

[ChrisRackauckas]

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Devin CLI 3000.11.3, model fusion-claude-opus-5-5-high-sidekick-swe-2-medium): MERGE, risk low.

Full review

VERDICT: MERGE
RISK: low

Blocking findings

None.

Non-blocking findings

1. The scalar classification logic now exists twice (suspected from reading). src/symbol_cache.jl:151-159 (get_all_timeseries_indexes(sc, sym)) and src/symbol_cache.jl:167-174 (_accumulate_timeseries_indexes!(acc, sc, sym)) contain the same is_variable || is_independent_variable / is_timeseries_parameter branches. If someone later changes one of them, for example to add a new timeseries kind, array inputs and scalar inputs will quietly give different answers.

   • The duplication is defensible. Turning the scalar method into a thin wrapper over the accumulator would change its return type from Set{ContinuousTimeseries} to Set{Any} for variables.
   • It is still worth a follow-up: add a test that checks get_all_timeseries_indexes(sc, [s]) == get_all_timeseries_indexes(sc, s) for each kind of symbol.

2. Array elements no longer go through get_all_timeseries_indexes dispatch (suspected from reading; no affected caller found).

   • On master, each array element was dispatched through get_all_timeseries_indexes(sc, el).
   • Now, any element that is not an Expr or an AbstractArray goes to the private _accumulate_timeseries_indexes! fallback. A downstream method get_all_timeseries_indexes(::SymbolCache, ::OwnedType) would therefore be ignored for array inputs.
   • I searched the local depot copies of ModelingToolkit*, SciMLBase, RecursiveArrayTools, Symbolics, SymbolicUtils, OrdinaryDiffEq*, JumpProcesses, Catalyst and DiffEqBase. The only extension I found is on SciMLBase's test-only NumSymbolCache, a separate wrapper type that this change does not affect. Downstream CI (MTK, RAT, Symbolics) is green.

3. The allocation test depends on inlining (confirmed by running).

   • With --inline=no on Julia 1.11, the head code allocates 606 times at N=100, so @test allocs <= 16 would fail.
   • With --code-coverage=user --check-bounds=yes, which matches the CI coverage setup, it stays at 6 allocations on both 1.10 and 1.11. Normal CI is fine. This only matters if someone runs the suite with inlining disabled.

4. PR body (no action needed).

   • It links no issue, so there is no tracking issue to check sibling work against. No other open PR touches src/symbol_cache.jl or test/symbol_cache_test.jl; the open PRs are 83, 84, 113 and 187-192.
   • The one red check, "SciMLBase.jl/SymbolicIndexingInterface Downstream", is a pre-existing failure. It fails the same way on master (run 37208940693 at b0724b8: FieldError: type SciMLBase.SCCNonlinearProblem has no field u0), so this PR did not cause it.
   • The repo root has no AGENTS.md or CLAUDE.md, so I applied only the global rules. The PR adds no dependencies, no new public names (the helper has a _ prefix and is not exported), no comments, and covers a single concern.

What I ran

All runs were in scratch/, against two environments: envhead (dev'd on the PR checkout) and envmaster (dev'd on git archive origin/master).

• Equivalence probe (scratch/equiv.jl), confirmed by running.

  • Inputs: 310 inputs (10 hand-picked plus 300 random nested Any[] arrays, depth ≤ 2). Elements were drawn from states, parameters, timeseries parameters in two different timeseries, the independent variable, unknown symbols, numbers, Exprs, ParameterTimeseriesIndex values, tuples, nothing and strings. Shapes included a matrix, a 3-D array and a range.
  • SymbolCaches: four of them, namely a full one with timeseries parameters, one with no independent variable, an empty SymbolCache(), and one with no timeseries parameters.
  • Comparison: element type plus sorted contents, on head vs master.
  • Result: diff eq_head.txt eq_master.txt gave IDENTICAL across 1240 lines with 0 errors. The outputs are non-trivial; they include sets such as {1,2,3,ContinuousTimeseries()}.

• Fails before, passes after (confirmed by running), using the PR's test/symbol_cache_test.jl included under @testset.

  • Head source: 88/88 pass on Julia 1.11, and 88/88 on Julia 1.10 with --code-coverage=user --check-bounds=yes.
  • Master source on 1.11: Test Failed at test/symbol_cache_test.jl:92, Evaluated: 1502 <= 16 (87 pass, 1 fail).
  • Master source on 1.10: 1405 allocations.
  • The new mixed assertions are hand-derived: :x gives Continuous, :b gives index 1, :(x + a) gives Continuous. They are not copied from the code's output, and they also pass on master, as expected for a refactor.

• Allocation counts (scratch/alloc.jl, @allocations through a @noinline barrier), confirmed by running:

  Run	N=100	N=1000	mixed
  head, 1.11	6	6	5
  head, 1.11 --check-bounds=yes	6	6	5
  head, 1.11 --code-coverage=user --check-bounds=yes	6	6	5
  head, 1.11 --inline=no	606	6006	54
  head, 1.10 (plain and with coverage)	6	6	5
  master, 1.11	1502	15002	73

  The <= 16 bound does not grow with N, so it really catches the per-element allocation regression.

• Benchmarks (scratch/bench.jl, BenchmarkTools minimum, Julia 1.11, M2 Max). The speedup reproduces at a similar ratio to the PR body's CI-box numbers.

  Call	Master	Head
  get_all_timeseries_indexes, N=1000	274.8 µs, 15000 allocs	14.5 µs, 4 allocs
  getu, N=1000	293.6 µs	31.2 µs
  get_all_timeseries_indexes, N=100	27.2 µs	1.39 µs

• Formatting and spelling: julia +1.12 --project=@runic -m Runic --check --diff src/symbol_cache.jl test/symbol_cache_test.jl passed with no diff, and typos on both files was clean.

• CI: gh pr checks 191 shows every lane green (Core 1/lts/x86/pre, QA, Downstream 1/lts, Docs, Runic, typos, MTK/RAT/Symbolics downstream) except the pre-existing SciMLBase downstream failure described above.

What I did not verify

• I did not run the full GROUP=Core or GROUP=QA locally, because of the host limits. I relied on the green CI lanes for those, and for the 32-bit lane.
• I did not run the downstream packages locally beyond the CI results above.
• The author's CI-box timing numbers are not reproducible here; I reproduced the ratio on different hardware.

---

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.3 (review), Claude Code 2.1.285 (posting) · model: fusion-claude-opus-5-5-high-sidekick-swe-2-medium
Conversation: local Claude session 3c311569-dda6-4687-bc9f-65febb212c33 (fleet master)

[ChrisRackauckas-Claude]

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

Independent review (Codex CLI 0.166.0 / gpt-6.1-sol), session 01a196cb-a253-7af3-bf77-007f09146dd8, reviewed head 522f0dd

• Verdict: MERGE
• Risk: medium-low (ML3: array collection drops per-element temporary sets; independent result checks and measurements verify the optimization on this peripheral SymbolCache path)
• Round / slot: 1 / 1
• Blocking priorities: none

Review written by the reviewer named above in a session other than the PR author's, and copied here by the posting agent without change to its findings. The posting agent did not re-run the reviewer's commands.

Summary: This PR reduces allocations when collecting SymbolCache timeseries indexes while preserving custom scalar dispatch and legitimate nothing indexes. Focused head/base execution and an independent contract probe support merging; the observed CI failures also occur in completed master runs.

Blast radius: SymbolCache implementations of get_all_timeseries_indexes for scalars and AbstractArray inputs, including calls from getsym/getu and parameter indexing. Expr collection retains its existing implementation; custom non-Symbol methods still dispatch through the public function.

Evidence: Head test file: 114/114 passed, exit 0. Identical head test file against the merge-base: 113 passed, one allocation assertion failed, exit 1 (1500 <= 16). Independent declared-index oracle: 265/265 passed, exit 0. For 1000 variable symbols, the merge-base algorithm reference used 470.6 microseconds / 15000 allocations versus head 22.6 microseconds / 4 allocations. Core LTS/prerelease and several integration checks pass; failing stable-Julia lanes and SciMLBase integration failures have matching master log signatures. Blind risk was medium (M3); reviewer measurements supply the evidence for ML3.

Blocking findings

None.

Non-blocking findings

None.

What I ran

All three Julia invocations used Julia 1.13.2 on aarch64, one process at a time, four Julia threads, one OpenBLAS thread, an isolated temporary directory, and the designated shared depot followed by the default depots. Directory values below are represented by environment variables to avoid machine-specific paths. Temporary environments and scripts were outside the source checkout; no package suite or Pkg.test() was run.

git fetch origin '+refs/pull/191/head:refs/remotes/pr/191' master
git checkout --detach pr/191
git rev-parse HEAD
git diff --stat origin/master...HEAD
git diff origin/master...HEAD
git merge-base origin/master HEAD
git log -1 --no-merges --format=%B
git archive b0724b8d8fbb8cecac9c2f1ebbcb4022067feaff

These successful git commands established the target head and merge-base, inspected the entire two-file diff, extracted a separate base snapshot, and read the newest non-merge commit trailers. The live PR was OPEN and draft. Source inspection included src/symbol_cache.jl:142, src/index_provider_interface.jl:137, src/state_indexing.jl:238, src/state_indexing.jl:361, and the parameter-indexing callers. No repository AGENTS.md or CLAUDE.md was present. The blind assessment was written and hashed before reading author context.

JULIA_NUM_THREADS=4 OPENBLAS_NUM_THREADS=1 TMPDIR="$REVIEW_TMPDIR" JULIA_DEPOT_PATH="$SHARED_DEPOT:" JULIA_PKG_PRECOMPILE_AUTO=0 timeout 3600 julia +1.13.2 --startup-file=no run-head.jl
JULIA_NUM_THREADS=4 OPENBLAS_NUM_THREADS=1 TMPDIR="$REVIEW_TMPDIR" JULIA_DEPOT_PATH="$SHARED_DEPOT:" JULIA_PKG_PRECOMPILE_AUTO=0 timeout 3600 julia +1.13.2 --startup-file=no run-base.jl
JULIA_NUM_THREADS=4 OPENBLAS_NUM_THREADS=1 TMPDIR="$REVIEW_TMPDIR" JULIA_DEPOT_PATH="$SHARED_DEPOT:" JULIA_PKG_PRECOMPILE_AUTO=0 timeout 3600 julia +1.13.2 --startup-file=no --project=env-head probe.jl

The first two scripts each activated their own temporary environment, used Pkg.develop(path=...) for the head or archived merge-base, and called Pkg.instantiate(; allow_autoprecomp=false). Each included the complete head version of test/symbol_cache_test.jl:1 inside a Test testset.

Head, exit 0:
PR head symbol_cache_test | 114  114
Base, exit 1:
Expression: allocs <= 16
Evaluated: 1500 <= 16
Merge-base with PR symbol_cache_test | 113 passed, 1 failed, 114 total
Third run, exit 0:
Independent declared-index oracle | 265  265

The independent oracle used an explicit symbol-to-index mapping and expression/array traversal without the changed helper. It checked symbols, integer indices, expressions, mixed and nested arrays, matrices, views, zero-dimensional and empty arrays, a downstream-owned scalar method, duplicate indexes, fresh-result isolation, and default empty-cache behavior. Index values included nothing, missing, Some(nothing), Some(7), symbols, strings, integers, tuples, and NaN. Expected values came from the declared mapping, not captured implementation output.

The third run also compared the head with the merge-base scalar and array algorithms transcribed into separate reference functions. After warmup, it measured allocations/bytes and the minimum of 20 batches of 100 calls:

Input	Base reference time	Head time	Base allocations / bytes	Head allocations / bytes
1000 variable symbols	470.6 us	22.6 us	15000 / 672064	4 / 320
1000 integer indices	1650.8 us	1351.8 us	15000 / 672064	7004 / 272320
Scalar variable	96.8 ns	98.8 ns	7 / 272	7 / 272
Scalar nothing index	104.4 ns	108.8 ns	7 / 272	7 / 272

The added allocation regression test fails on the merge-base and passes on the head in my own runs. All added functional assertions, including the nothing-index and custom-dispatch assertions, already pass on the merge-base: the net PR is a performance improvement preserving those contracts. I did not separately run the intermediate PR revisions that introduced and then repaired those behaviors.

CI inspection commands:

gh pr view 191 --repo SciML/SymbolicIndexingInterface.jl --json number,url,state,isDraft,headRefOid,baseRefName,baseRefOid,files,statusCheckRollup
gh pr checks 191 --repo SciML/SymbolicIndexingInterface.jl
gh run list --repo SciML/SymbolicIndexingInterface.jl --branch master --event push --limit 5 --json databaseId,headSha,conclusion,url,name
gh run list --repo SciML/SymbolicIndexingInterface.jl --branch master --event push --workflow Tests.yml --limit 5 --json databaseId,conclusion,url,headSha
gh run list --repo SciML/SymbolicIndexingInterface.jl --branch master --event push --workflow Downstream.yml --limit 5 --json databaseId,conclusion,url,headSha
gh run view 38893435297 --repo SciML/SymbolicIndexingInterface.jl --log-failed
gh run view 38893435080 --repo SciML/SymbolicIndexingInterface.jl --log-failed
gh run view 38854325363 --repo SciML/SymbolicIndexingInterface.jl --log-failed
gh run view 38854325185 --repo SciML/SymbolicIndexingInterface.jl --log-failed

gh pr checks exited 1 because of failed jobs; the metadata/list/log commands succeeded. Initial log retrieval could not write the default CLI cache; setting XDG_CACHE_HOME to the isolated temporary directory allowed retrieval.

PR Tests run: LTS and prerelease Core/Downstream lanes pass. Stable Julia Core, x86 Core, Downstream, and QA fail before assertions because JET 0.9-0.11 conflicts with Julia 1.13's requirement for JET 0.12. The same four resolution failures appear in the completed master Tests run.

PR IntegrationTest run: ModelingToolkit, Symbolics, and RecursiveArrayTools pass. SciMLBase reports 3747 passed and two errors: State indexing and Observed in test/downstream/comprehensive_indexing.jl:127 and test/downstream/comprehensive_indexing.jl:164, both BoundsError accessing a one-element vector at indices [1, 2]. Both exact test locations and exceptions also appear in the completed master IntegrationTest run. These are verified inherited failures, not a claim that integration is green. Documentation, formatting, downgrade, spellcheck, and benchmark checks report success.

What I did not verify

I did not run full Core, QA, downstream, documentation, or benchmark suites locally, nor Julia LTS, prerelease, x86, GPU, AD, MPI, or Reactant workloads. No GPU kernel, AD rule, MPI path, tracing path, shared state, cache, or numerical algorithm is changed; G1-G3 introduce no additional unmet gate for this diff. I did not reproduce the author's exact historical timings or benchmark getu end-to-end; the measurements above cover direct collection and scalar fallback behavior on the review environment. Intermediate PR revision regressions were not independently executed.

The PR-description filter accidentally exposed a one-line older independent verdict after the blind risk assessment had been hashed; the full prior review remained unread until this review was completed and frozen. No PR comments, batch comments, or other reviewer directories were read, and no upstream writes were made.

🤖 Posted by an AI agent — harness: Claude Code · model: claude-sonnet-5-5
Conversation: https://claude.ai/code/session_01379heiiB35TtywhR7uSU74