Skip to content

Namespace indexed array slices without renamespacing the getindex call - #5133

Draft
ChrisRackauckas wants to merge 3 commits into
masterfrom
fix-array-slice-namespacing
Draft

ChrisRackauckas wants to merge 3 commits into
masterfrom
fix-array-slice-namespacing

Conversation

@ChrisRackauckas

@ChrisRackauckas ChrisRackauckas commented Sep 13, 2026 •

Copy link
Copy Markdown
Member

🚧 UNREVIEWED WORK IN PROGRESS — awaiting review by @ChrisRackauckas. Pushed by an AI agent running as @ChrisRackauckas so the work is not lost; Chris has not reviewed it. Please ignore this draft until he does.
Harness: Claude Code 2.1.270 · Model: claude-fable-5-1
Conversation: local Claude Code session 9b5c3ae5-6a0f-41b8-bd69-03bf3b5e7083 (no shareable URL available)

namespace_expr called renamespace on every getindex term that indexes a variable. When the indexed parent is an array slice (an ArrayOp, e.g. the element expression of a broadcast over U[k, :, :]), renamespace has no matching branch and flattening a subsystem fails with matching non-exhaustive. For getindex the call was a no-op (the arguments were already namespaced and only f/metadata were taken from the result), so it is skipped.

Evidence (Julia 1.12.6, aarch64, fresh processes; lib/ModelingToolkitBase/test/array_slice_namespacing.jl):

master:  Broadcasts over array slices namespace through subsystems | Error 1 / 1   (matching non-exhaustive, renamespace)
branch:  Broadcasts over array slices namespace through subsystems | 3 passed / 3

Not verified: the InterfaceI group run was started but not finished, QA, x86. Registered in runtests.jl after "Variable Scope Tests".


🤖 Posted by an AI agent — harness: Claude Code 2.1.270 · model: claude-fable-5-1
Conversation: local Claude Code session 9b5c3ae5-6a0f-41b8-bd69-03bf3b5e7083 (no shareable URL available)

Risk assessment (independent review, codex-cli gpt-6-astra, reviewed head 34b88bc)

  • Risk: low
  • Blast radius: Subsystem expression namespacing for indexed symbolic variables in ModelingToolkitBase. One condition avoids a redundant renamespace(getindex(...)) call; no public declaration/signature changes, dependency changes, tolerance changes, or removed coverage.
  • Evidence: Reviewed head 34b88bc3db50d18d2c21ec4e8c4b036c5630beab, unchanged at final recheck. On Julia 1.13.0 with four threads, the new regression passed 3/3 and existing variable-scope tests passed 44/44. Restoring the original method and rerunning the identical new regression produced matching non-exhaustive (1 error). CI also reports the new test passing 3/3 on release and LTS: https://github.com/SciML/ModelingToolkit.jl/actions/runs/38850827482/job/116812083464 and https://github.com/SciML/ModelingToolkit.jl/actions/runs/38850827482/job/116812083620 . Final CI snapshot: 100 passed, 7 failed, 1 pending, 1 skipped; failure analysis below.
  • Independent review: codex-cli 0.163.0 gpt-6-astra rated it low, high confidence: the changed call's only consumed outputs are unchanged operation/metadata, and the regression discriminates the fix while existing scope tests pass.
  • Merge: needs human review — no code change requested, but raw CI remains red and the unique stochastic downstream failure has been isolated from the changed path rather than reproduced on the default branch. MERGE is the code-review verdict, not authorization to bypass required checks.

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

🤖 Automated review section from an AI agent running as Chris Rackauckas's account — not written or reviewed by Chris.

Risk assessment (independent review, Codex gpt-6.1-sol, head b79d21e)

  • Verdict: CHANGES
  • Rating: Codex gpt-6.1-sol rated it low (L6) at head b79d21e
  • Risk: low (L6: fixes an error on broadcasts over array slices, with independently reproduced failing-before/passing-after evidence; successful indexed-expression outputs are unchanged). L2 also applies to the added regression and test registration; no higher criterion matched.
  • Blast radius: Indexed symbolic expressions during subsystem equation namespacing, including flattening/connection expansion. The getindex branch of renamespace returns the original operation and metadata, which are the only outputs consumed here; recursive argument namespacing remains intact. No public signatures, defaults, dependencies, or numerical algorithms change.
  • Evidence: Read the full git diff origin/HEAD...HEAD, issue 5137 and tracking issue 5139. git diff --check origin/HEAD...HEAD passed. Scratch copies came from git archive HEAD lib/ModelingToolkitBase and git archive origin/HEAD lib/ModelingToolkitBase; default head was 808a4f1. Used Julia 1.12.4, Symbolics 7.46.0, SymbolicUtils 4.51.0, the requested depot, four threads, serial Julia invocations, and timeout 3600. Head julia --project=<local path> -e '…' included array_slice_namespacing.jl and variable_scope.jl: regression 3/3, scope file completed without failures, exit 0. Base julia --project=<local path> -e '…' included the unchanged head regression against the base package: matching non-exhaustive in renamespace, 1 error, exit 1; its existing scope tests passed 44/44. julia --project=<local path> <local path> and the same script included in the base invocation produced 7 identical printed indexed-expression snapshots, covering local/parent/global scope, parameters, derivatives and compound expressions. Runtime Base.ispublic(Symbolics, :getname) returned false; arguments and value returned true. Logs: head, base, public API/output check.
  • Evidence: All three failed PR checks have matching default-branch log evidence: documentation curl timeout to OpenModelica (exit 28), SciMLBase comprehensive_indexing.jl:127/:164 BoundsErrors (3747 passed, 2 errored on both), and SciMLSensitivity MTK Forward Mode mtk.jl:216 with the same underlying 3600-element vector indexed at 3601. Historical CI does not establish an introducing commit; documentation and Core8 have intermittent successes. The read-only baseline investigation is recorded here.
  • Evidence: G1 not applicable — no numerical algorithm/results change on a working path. G2 not applicable — no GPU kernel, AD rule, Reactant tracing or MPI path is modified; the changed namespacing path was exercised locally and by InterfaceI CI. G3 not applicable — no shared state, cache policy, threading or global default change. No tests are skipped or loosened, no dependency/license change is introduced, and no new public name requires documentation or a SemVer bump.
  • Merge: needs changes (finding 1: use the public owner of getname in the new test). No functional defect in the namespacing condition was found.

🤖 Posted by an AI agent — harness: Codex CLI 0.162.1 · model: gpt-6.1-sol
Conversation: local headless review run (no public session URL); transcript kept by the fleet master.

`namespace_expr` renamespaced every variable-like term, including
`getindex` calls. For an array slice such as `U[k, :, :]` inside a
subsystem, `renamespace` has no matching branch and fails with
`matching non-exhaustive`. The parent array has already been namespaced
through the recursive argument pass, so an indexed term only needs its
arguments rebuilt; skip `renamespace` when the operation is `getindex`.

Adds `test/array_slice_namespacing.jl`, which builds a subsystem whose
equations broadcast over `U[k, :, :]` and checks the flattened system's
namespaced unknowns and equation variables.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.270
Agent-Model: claude-fable-5-1
Agent-Session: local Claude Code session 9b5c3ae5-6a0f-41b8-bd69-03bf3b5e7083
@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Benchmark Results (Julia v1)

Time benchmarks
master b79d21e... master / b79d21e...
ODEProblem 4.02 ± 0.21 ms 4.05 ± 0.26 ms 0.993 ± 0.082
init 0.0534 ± 0.022 ms 0.0564 ± 0.013 ms 0.946 ± 0.45
large_parameter_init/ODEProblem 11.6 ± 1.4 ms 13.1 ± 2.4 ms 0.882 ± 0.19
large_parameter_init/init 0.0541 ± 0.0041 ms 0.0594 ± 0.029 ms 0.911 ± 0.44
mtkcompile 7.4 ± 0.89 ms 7.68 ± 1 ms 0.963 ± 0.17
sparse_analytical_jacobian/ODEProblem 13.2 ± 1.6 ms 15.2 ± 2 ms 0.867 ± 0.16
sparse_analytical_jacobian/f_iip 0.05 ± 0.01 μs 0.05 ± 0.01 μs 1 ± 0.28
sparse_analytical_jacobian/f_oop 0.0847 ± 0.013 ms 0.0852 ± 0.012 ms 0.994 ± 0.21
time_to_load 4.25 ± 0.04 s 4.33 ± 0.045 s 0.981 ± 0.014
Memory benchmarks
master b79d21e... master / b79d21e...
ODEProblem 0.0333 M allocs: 1.59 MB 0.0333 M allocs: 1.59 MB 1
init 0.477 k allocs: 0.0602 MB 0.477 k allocs: 0.0602 MB 1
large_parameter_init/ODEProblem 0.139 M allocs: 4.61 MB 0.139 M allocs: 4.61 MB 1
large_parameter_init/init 0.759 k allocs: 0.166 MB 0.759 k allocs: 0.166 MB 1
mtkcompile 0.058 M allocs: 2.68 MB 0.058 M allocs: 2.68 MB 1
sparse_analytical_jacobian/ODEProblem 0.102 M allocs: 4.23 MB 0.102 M allocs: 4.23 MB 1
sparse_analytical_jacobian/f_iip 0 allocs: 0 B 0 allocs: 0 B
sparse_analytical_jacobian/f_oop 0.848 k allocs: 27 kB 0.848 k allocs: 27 kB 1
time_to_load 0.201 k allocs: 12.1 kB 0.201 k allocs: 12.1 kB 1

@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Benchmark Results (Julia vlts)

Time benchmarks
master b79d21e... master / b79d21e...
ODEProblem 7.16 ± 1.6 ms 5.72 ± 0.37 ms 1.25 ± 0.28
init 0.121 ± 0.046 ms 0.113 ± 0.043 ms 1.07 ± 0.58
large_parameter_init/ODEProblem 20.1 ± 6.2 ms 24.9 ± 5.1 ms 0.809 ± 0.3
large_parameter_init/init 0.124 ± 0.049 ms 0.167 ± 0.059 ms 0.743 ± 0.39
mtkcompile 13 ± 3.5 ms 11.3 ± 1.1 ms 1.15 ± 0.33
sparse_analytical_jacobian/ODEProblem 27.9 ± 5.4 ms 29.2 ± 4.6 ms 0.959 ± 0.24
sparse_analytical_jacobian/f_iip 0.07 ± 0 μs 0.07 ± 0.01 μs 1 ± 0.14
sparse_analytical_jacobian/f_oop 0.368 ± 0.015 ms 0.377 ± 0.023 ms 0.975 ± 0.07
time_to_load 8.91 ± 1.2 s 11.3 ± 1.4 s 0.791 ± 0.15
Memory benchmarks
master b79d21e... master / b79d21e...
ODEProblem 0.0343 M allocs: 1.79 MB 0.0343 M allocs: 1.79 MB 1
init 0.486 k allocs: 0.0779 MB 0.486 k allocs: 0.0779 MB 1
large_parameter_init/ODEProblem 0.145 M allocs: 5.42 MB 0.145 M allocs: 5.42 MB 1
large_parameter_init/init 0.618 k allocs: 0.172 MB 0.618 k allocs: 0.172 MB 1
mtkcompile 0.0619 M allocs: 3.43 MB 0.0619 M allocs: 3.43 MB 1
sparse_analytical_jacobian/ODEProblem 0.106 M allocs: 5.32 MB 0.106 M allocs: 5.32 MB 1
sparse_analytical_jacobian/f_iip 0 allocs: 0 B 0 allocs: 0 B
sparse_analytical_jacobian/f_oop 0.634 k allocs: 19.6 kB 0.634 k allocs: 19.6 kB 1
time_to_load 0.153 k allocs: 14.6 kB 0.153 k allocs: 14.6 kB 1

ChrisRackauckas and others added 2 commits October 16, 2026 04:28
…espacing

Pulls in master's fixes for the Symbolics 7.43+ scalar-default check
(variable_parsing.jl) and the Symbolics 7.45/Latexify 0.16.6 align
output (latexify/50.tex) from #5281, which resolve the InterfaceI
"Parsing Test" and RegressionI "Latexify recipes Test" failures seen
on this branch's CI after those upstream releases.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Devin <noreply@cognition.ai>
Agent-Harness: Devin CLI 3000.11.92
Agent-Model: swe-2-max
Agent-Session: local Devin CLI session d2affd818d31733f9bd27d101eea94bd
@ChrisRackauckas

Copy link
Copy Markdown
Member Author

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

The branch had gone stale: upstream releases (Symbolics ≥7.43, Latexify ≥0.16.6) broke two testsets on this branch's CI, and master had since moved on with the fixes.

What was wrong

  • sublibrary-ci / lib/ModelingToolkitBase [InterfaceI] (Julia 1 + lts): "Parsing Test" errored with ArgumentError: Variable x must have default of matching size. Got [1, 2] with size (2,) — Symbolics ≥7.43's setdefaultval rejects a scalar @variables x = [1, 2] in variable_parsing.jl.
  • sublibrary-ci / lib/ModelingToolkitBase [RegressionI] (Julia 1 + lts): "Latexify recipes Test" errored — latexify/50.tex reference no longer matched Symbolics 7.45 align output / Latexify ≥0.16.6.
  • Both were dependency-drift failures (resolved Symbolics 7.46.0 / Latexify 0.16.12), already fixed on master by Update MTKBase tests and compat floors for Symbolics 7.43-7.45 #5281 — not caused by this PR.

What I changed

Validation (Julia 1.13.2, Symbolics 7.46.0, Latexify 0.16.12 — same versions as the failed CI jobs)

  • GROUP=ModelingToolkitBase_InterfaceI JULIA_NUM_THREADS=4 JULIA_NUM_PRECOMPILE_TASKS=4 julia +release -e 'using Pkg; Pkg.test("ModelingToolkitBase")' → InterfaceI: 1806 passed, 5 broken, 0 failed/errored (10m36s), including the new "Array Slice Namespacing" testset (3/3) and the previously-erroring "Parsing Test".
  • Same command with GROUP=ModelingToolkitBase_QA → JET: 54/54, Aqua: 21/21.
  • julia +release --project=@runic -m Runic --inplace <changed .jl files> → no changes needed.
  • typos over the diff → clean.

Pre-existing failures left alone (fail identically on master, not caused by this PR)

  • SciMLBase.jl/SymbolicIndexingInterface/1: BoundsError in downstream comprehensive_indexing.jl; master's IntegrationTest run (38950292025) fails the same job.
  • build (Documentation): makedocs linkcheck curl failure on an external link; master's Documentation run fails too.
  • Catalyst/Hybrid/1: one stochastic solver tolerance miss at hybrid_models.jl:975 (single element >5% of reference); unrelated to namespacing.
  • downgrade-mtkbase (Optimization): canceled at the 6h timeout mid-Ipopt-solve; the group is documented as exceeding timeouts on shared runners.

🤖 Posted by an AI agent — harness: Devin CLI 3000.11.92 · model: swe-2-max
Conversation: local Devin CLI session d2affd818d31733f9bd27d101eea94bd

@xtalax

xtalax commented Oct 19, 2026

Copy link
Copy Markdown
Member

Tested on head b79d21e (SymbolicUtils 4.52.0). A slice-form system (D(u[2:4]) ~ u[1:3] .- 2 .* u[2:4] .+ u[3:5]) used as a subsystem now compiles with mtkcompile, solves with Tsit5, and matches the flat system's solution (2/2). On master it fails with matching non-exhaustive in renamespace. Calling renamespace(sys, u[2:3]) directly on a slice still errors here, but mtkcompile doesn't take that path.

🤖 Posted by an AI agent · harness: Claude Code 2.1.304 · model: claude-opus-5-5
Review: unreviewed
Session: aa65e380-52df-443a-9f57-8c20c56babb9

@ChrisRackauckas-Claude

ChrisRackauckas-Claude commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

🤖 Automated comment from an AI agent — not written or reviewed by Chris Rackauckas.

The independent review found the namespacing behavior correct, with one test API qualification blocker: Symbolics.getname is an imported, non-public binding; its public owner is SymbolicIndexingInterface. The reviewer executed these checks on the latest and effective lower dependency combinations and confirmed the two spellings refer to the same function.

Proposed patch against b79d21e493d916355e84efcef30170f38f629ca8:

--- a/lib/ModelingToolkitBase/test/array_slice_namespacing.jl
+++ b/lib/ModelingToolkitBase/test/array_slice_namespacing.jl
@@ -1,4 +1,5 @@
 using ModelingToolkitBase, Symbolics, Test
+using SymbolicIndexingInterface: getname
 using ModelingToolkitBase: t_nounits as t, D_nounits as D
 
 @testset "Broadcasts over array slices namespace through subsystems" begin
@@ -10,11 +11,11 @@
     @named outer = System(Equation[], t; systems = [cell])
     flat = flatten(expand_connections(outer))
     @test length(equations(flat)) == 3
-    namespaced = Set(Symbolics.getname.(unknowns(flat)))
+    namespaced = Set(getname.(unknowns(flat)))
     @test namespaced == Set([:cell₊U, :cell₊I])
     rhs = equations(flat)[end].rhs
     variables = Set(
-        Symbolics.getname(first(Symbolics.arguments(Symbolics.value(v))))
+        getname(first(Symbolics.arguments(Symbolics.value(v))))
             for v in Symbolics.get_variables(rhs)
     )
     @test variables == Set([:cell₊U])

git apply --check passed. The exact qualification patch has now been executed in isolated copies of this unchanged head on both latest and effective-floor dependencies; all original assertions and expected values are preserved. Full registered InterfaceI should still be rerun after author adoption. The review ran the original test as written: its three assertions pass on the head and construction fails on master (matching non-exhaustive in renamespace). The complete focused head run passes 168/168; 110 previously working expressions and their metadata compare equal (220/220), with seven further independent slice assertions. Those checks also pass with Symbolics 7.45.0 / SymbolicUtils 4.46.4.

The public owning dependency already exists. This proposal changes only the new test's import and two calls; no author branch was pushed. Full QA/InterfaceI, actual GPU execution, and complete PointPort composition were not validated in this review.

Links:

Validation update (2026-10-21 08:06 UTC):

Julia Symbolics / SymbolicUtils Entire patched namespacing test Public-owner/identity checks
1.12.7 7.47.0 / 4.53.0 3/3 passed, exit 0 3/3 passed
1.12.7 7.45.0 / 4.46.4 3/3 passed, exit 0 3/3 passed

The binding checks verify identity with SymbolicIndexingInterface.getname, its public declaration at that owner, and its parent module. Both archives use MTKBase 1.77.4 from this PR and SII 0.3.55. Patched file bytes are identical between environments. Whole-file Runic 1.10.0, spelling over added lines, and whitespace-strict apply check all exit 0. The original three functional assertions already passed before this qualification change; this addresses public dependency ownership, not a numerical defect. Full QA/InterfaceI/docs/downstream/GPU/composed-MOL execution were not repeated by these focused runs.

Validation: Codex CLI 0.163.0 / gpt-6.1-sol, local session 01a15cab-4877-7682-9c4e-147cebaecd19 (no session URL). Live head was rechecked after the runs and remained b79d21e4. No author branch was changed.

🤖 Posted by an AI agent — harness: Codex CLI 0.163.0 · model: gpt-6.1-sol. Independent reviewer: Codex CLI 0.163.0 / gpt-6.1-sol, local session 01a15d13-db8a-70a1-9610-1b094d84b971 (no session URL).
Conversation: local session 01a15ca3-5696-7ea3-bbac-ba551b6975af (no session URL exposed).

xtalax added a commit to xtalax/MethodOfLines.jl that referenced this pull request Oct 21, 2026
Master's slice broadcasts are now spelled as a non-inlined mol_broadcast call
(#717), so a discretized system nests inside another system on released
ModelingToolkitBase without SciML/ModelingToolkit.jl#5133: after merging master
the gated composition test passed unexpectedly. Drop the namespacing probe and
its @test_broken gate so every composed solve runs, and drop the #5133
requirement from the docs and the PointPort docstring.

Connectors on released MTK: 51/51.

Agent-Harness: Claude Code 2.1.306
Agent-Model: claude-opus-5-5
Agent-Session: fe6e5fd4-da2b-43af-8f0d-e3c4ab662c0d
Agent-Review: unreviewed
Co-authored-by: Claude Code <agent@noreply.juliahub.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-generated Filed by an AI agent needs-chris-review Bot-filed finding awaiting review by ChrisRackauckas

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants