Repository navigation
Keep spelled NonlinearSolution element type concrete across SciMLBase layouts - #1332
Draft
ChrisRackauckas-Claude wants to merge 3 commits into
Draft
ChrisRackauckas-Claude wants to merge 3 commits into
ChrisRackauckas-Claude wants to merge 3 commits into
Conversation
This was referenced Sep 26, 2026
…Base layouts
`iteratively_build_sols(probs::AbstractVector)` spells out
`NonlinearSolution{...}` to type the `sols` buffer, and the vector-form
test spells the same type into the `FunctionWrapper` `SubArray` signature.
SciMLBase #1620 adds a trailing `RC` type parameter to `NonlinearSolution`,
which turns that application into a UnionAll: `Vector{ST}` then has an
abstract eltype and `isconcretetype(eltype(original))` fails.
`stripped_solution_type` spells the ten non-`RC` parameters and applies
`ReturnCode.T` only when the spelled result is not already concrete, so
`ST` is the concrete stripped-solution type under both the released
SciMLBase layout and the #1620 layout. Verified: GROUP=Core passes
against SciMLBase 3.56.0 and against the #1620 branch, including the
vector-form concreteness assertion.
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Devin <noreply@cognition.ai>
Agent-Harness: Devin CLI 3000.11.3
Agent-Model: swe-2-high
Agent-Session: local session, transcript at /home/crackauc/sandbox/agent-jobs/SciMLBase.jl/jobs/1619-devin/log.txt on amdci2.julia.csail.mit.edu
Vector-form SCC users wrap explicitfuns with a FunctionWrapper whose
second argument spells SubArray{SSol,...}. Prefer that declared SSol
over the concrete stripped_solution_type so legacy ten-parameter
signatures keep converting under SciMLBase #1620's trailing RC param.
Add a regression test for the old signature next to the updated one.
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Cursor Agent <noreply@cursor.com>
Agent-Harness: Cursor Agent CLI 2026.09.26-dd393fe
Agent-Model: auto
Agent-Session: local session, transcript at /home/crackauc/sandbox/agent-jobs/NonlinearSolve.jl/jobs/1332-cursor/log.txt on amdci2.julia.csail.mit.edu
sols_buffer_eltype previously only inspected eltype/first, so Any[plain_noop, legacy_FW] still built a concrete buffer and broke conversion. Scan every element after the eltype fast path. Derive item4's SubArray element type from public strip_solution instead of the internal stripped_solution_type helper, and cover the mixed container in item8. Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com> Co-Authored-By: Cursor Agent <noreply@cursor.com> Agent-Harness: Cursor Agent CLI 2026.09.26-dd393fe Agent-Model: auto Agent-Session: local session, transcript at /home/crackauc/sandbox/agent-jobs/NonlinearSolve.jl/jobs/1332-cursor/log.txt on amdci2.julia.csail.mit.edu
ChrisRackauckas-Claude
force-pushed
the
agent/1619-scc-concrete-solution-type
branch
from
October 7, 2026 14:41
40bdc35 to
3104659
Compare
ChrisRackauckas-Claude
pushed a commit
to ChrisRackauckas-Claude/NonlinearSolve.jl
that referenced
this pull request
Oct 21, 2026
…, reinit!(u0,p)
Review (Codex, gpt-6.1-sol) on the init/step!/solve! cache found four correctness
issues, all confirmed by running probe scripts:
1. A second solve! on an already-completed cache recombined an empty history
(the fast "no stepping yet" path advanced idx past the end without
retaining any block solutions), returning a spuriously successful empty
u = Any[]. The fast path now builds the per-block solutions via
iteratively_build_sols (the same driver _scc_solve itself uses) and
retains them on the cache, so any later solve! - including after a
completion no-op step! - recombines the real result instead.
2. step! always passed Vector{Any} to explicitfuns!, breaking
tuple-specialized callbacks (MethodError) and the FunctionWrappers vector
path solve itself supports (conversion error). step! now passes the exact
representation the existing driver passes: a growing Tuple for tuple-form
blocks, a typed view into a concretely-typed, growing Vector for
vector-form blocks.
3. With store_original=Val(true) on vector-form blocks, stepping normalized
everything into a Tuple, so typeof(direct) != typeof(stepped). Fixed by
(2): cache.sols is now exactly the container _scc_solve/iteratively_build_sols
produce, so the shared __scc_combine_sols helper (extracted from
_scc_solve's tail, used by both the fast and stepped paths) always agrees
on original's container and element types.
The concrete per-block solution type for vector-form blocks is derived via
a new shared helper, __scc_vector_sol_type, factored out of the existing
vector-form iteratively_build_sols method rather than hand-spelled again -
deliberately, since the hand-spelled 10-parameter NonlinearSolution
signature is what core_tests__item4.jl's pre-existing failure traces to
(SciMLBase#1620 added an 11th type parameter; SciML#1332
is the fix for that, in progress separately). Sharing the helper means
this cache's types track the existing driver's, including through
whichever of SciML#1332 or a SciMLBase release lands first.
4. reinit! accepted no replacement u0/p. Added
reinit!(cache, u0 = missing; p = missing), delegating entirely to
SciMLBase.remake(cache.prob; u0, p) (which already knows how to chunk u0
across blocks and how parameters_alias governs distributing p) rather than
reimplementing that logic, then resetting progress on the result.
Also (non-blocking in review): declared SCCNonlinearCache public via
SciMLPublic.@public (the Julia 1.10-compatible mechanism SteadyStateDiffEq.jl
already uses for its analogous cache types), and bumped the minor version
(1.16.0 -> 1.17.0) for the new public API.
All four fixes verified against the reviewer's own probe scripts
(probes.jl, reinit_public.jl) before writing the corresponding assertions
into core_tests__item8.jl. Pre-existing tests (item1, item2, item3, item5,
item6, item7) rerun directly and still pass unchanged.
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
Agent-Harness: Claude Code 2.1.251 (subagent)
Agent-Model: claude-sonnet
Agent-Session: https://claude.ai/code/session_01NB3oFTNzGW79UtDt8AyjsM
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ignore until reviewed by @ChrisRackauckas.
What changed and why
iteratively_build_sols(probs::AbstractVector, ...)previously always typed thesolsbuffer with the concrete strippedNonlinearSolution(viastripped_solution_type). SciML/SciMLBase.jl#1620 adds a trailingRCtype parameter, so that concrete buffer no longer converts into aFunctionWrapperwhose signature still spells the ten-parameterSubArray{SSol,...}— SubArray is invariant in its element type.This is non-breaking: when any vector-form
explicitfunis aFunctionWrapperwhose second argument isSubArray{SSol,...}, SCC uses that declaredSSolas the buffer eltype (legacy ten-parameter spellings keep working, including under #1620 where that spelling is a UnionAll). The lookup keeps aneltypefast path for homogeneous wrapper vectors, then scans every element so a legacy wrapper that is not first (e.g.Any[plain_noop, legacy_FW]) still wins. Otherwise it uses the concrete stripped type so the updated-signature path keepsisconcretetype(eltype(original)). Numerically, step counts and iterates are unchanged.The updated-signature test derives its
SubArrayelement type from publicsolve+strip_solution+build_solution(not the internalstripped_solution_typehelper).Fail-before / pass-after
Homogeneous legacy wrappers (round 1)
Negative control on pre-compat src against SciMLBase #1620:
SubArrayconvertMethodError(5 pass, 1 error).Mixed
Any[plain, legacy FW](round 2, blocking)Negative control on
98947a2f(eltype/first-only scan) against SciMLBase #1620:After scanning every element (
40bdc35f),core_tests__item8.jl(incl. mixed case) passes 14/14 against both SciMLBase versions (below).Verification (Julia 1.12.7)
SciMLBase #1620:
SciMLBase released v3.56.0:
Lint/format:
Not verified
FunctionWrapper(or whose arg types do not expose aSubArray{NonlinearSolution,...}) — those still use the concrete stripped type.Reviewer push-back points
nameof(FW) === :FunctionWrapperplus.parametersrather than a hard dep on FunctionWrappers.jl.SSolis the ten-parameter UnionAll under #1620, the buffer eltype is intentionally non-concrete so the wrapper keeps converting; concreteness is asserted only in the updated-signature test (core_tests__item4.jl).Part of SciML/SciMLBase.jl#1619 (companion to SciML/SciMLBase.jl#1620).
Please ignore this draft until reviewed by @ChrisRackauckas.
Cursor Agent CLI 2026.09.26-dd393fe, model auto. Transcript: /log.txt
Adopted 2026-10-07 (ci-green head)
What changed: rebased onto master
0055e314(after #1321, #1356 and #1357). The rebase had no conflicts and made no code change. Head is now31046598. All original commits are kept.Where the fix belongs: SciMLBase v3.57.0 (SciML/SciMLBase.jl#1620, released) added the trailing
RCparameter on purpose. Julia has no default type parameters, so no constructor or alias in SciMLBase can make the ten-parameter spellingNonlinearSolution{T,…,Tr}concrete again. The fix has to come from SCC choosing a concrete buffer eltype, which is what this PR does. The SciMLBase PR body itself names this PR as its companion. ModelingToolkit's SCC codegen (src/problems/sccnonlinearproblem.jl) does not spell the solution type. The legacy FunctionWrapper path therefore serves user code that spells the ten-parameter type, such as the oldcore_tests__item4.jl.Verification:
GROUP=CorePkg.test()inlib/SCCNonlinearSolve, SciMLBase 3.57.0, on Julia 1.10.12 and 1.12.7.Master
0055e314fails on both versions:This branch passes.
Testing SCCNonlinearSolve tests passedon both versions:(The lts and 1 counts differ by one because of the
VERSION >= v"1.11"compile-time assertion.) Runic--checkandtyposon the changed files: clean.Red checks after the rebase (CI is still running):
benchmark (lts):Unsatisfiable requirements ... SciMLJacobianOperators restricted to 0.1.20 by NonlinearSolveFirstOrder. The benchmark env resolves from the registry, and SciMLJacobianOperators 0.1.20 (Convert Krylov Vector seeds to SVector for Enzyme VJP in SciMLJacobianOperators #1356) is not registered yet. It fails the same way on every open PR. This is infrastructure, not this PR.Risk assessment
iteratively_build_solsbuffer eltype selection, and only that. Tuple-form SCC is untouched, and no public API changes. The FunctionWrapper detection is structural: it checksnameofand theSubArrayparameters, with no FunctionWrappers dependency. An opaque wrapper falls back to the concrete stripped type.FunctionWrappersignatures converting, and dropping it would break those users.🤖 Generated with Claude Code (model: claude-opus-5-5[1m]) https://claude.ai/code/session_011aK5Xvp9NpdkzKuEwtiMGd
Independent review (added by the fleet master)
Verdict: MERGE (reviewed head 3104659)
SCCNonlinearProblemsolution buffers inSCCNonlinearSolve.iteratively_build_sols. The ten-parameterNonlinearSolution{T,N,uType,R,P,A,O,uType2,S,Tr}spelling is aUnionAllon current SciMLBase (the eleventh parameter is the retcode carrier).stripped_solution_typeappliesSciMLBase.ReturnCode.Twhen that spelling is not concrete. If anexplicitfunFunctionWrapperdeclares aSubArrayelement type, the buffer uses that declared type so the invariantSubArraystill converts. Tuple-form SCC is unchanged. No new dependency.SCCNonlinearSolvestays at 1.16.0. A concrete solution is still an instance of the old ten-parameter spelling, so this is a tighter element type, not a new solution struct.julia +release1.13.2 with SciMLBase v3.58.0: the ten-parameter spelling is not concrete; applyingReturnCode.Tis concrete. On masterdd8a550b79c4c9ea72a7b95b9233a34ffdb5bd8e,core_tests__item4.jlfailed one assertion and passed 12:isconcretetype(eltype(scc_sol_debug.original))was false forNonlinearSolution{Float64,1,Vector{Float64},Vector{Float64},@NamedTuple{p::Nothing},Nothing,Nothing,Nothing,Nothing,Nothing}. With head310465988d89acb6abee6f51bdae509cede8dc75sources,Pkg.testofSCCNonlinearSolvepassed every core file: item4 13, item8 (the new legacy ten-parameter wrapper, includingAny[plain, legacy]) 14, and items 1–3 and 5–7 (14, 3, 6, 7, 21, 11). CI sublibrary Core on Julia 1, lts, and pre, plus QA and the SCC downgrade job, passed: https://github.com/SciML/NonlinearSolve.jl/actions/runs/37638822372.benchmark (lts)failed while resolving, before any benchmark ran:NonlinearSolveFirstOrder2.10.0 requiresSciMLJacobianOperators0.1.20 and that run's registry only offered 0.1.0–0.1.19 (https://github.com/SciML/NonlinearSolve.jl/actions/runs/37638820774/job/112852033721).benchmark (1)passed. The three-dot diff does not touch FirstOrder or JacobianOperators. Compare vs master: 3 ahead, 3 behind (Bump PETSc in / #1329, Bump NonlinearSolveBase and SCCNonlinearSolve for release #1371, Allow JET 0.12 in the QA environment for Julia 1.13 #1373),MERGEABLE.SubArrayconversion still worksbenchmark (lts)job is a registry resolve rather than a result of this diff🤖 Section added by an AI agent — harness: Claude Code 2.1.300 · model: claude-opus-5-5 (review by grok-cli, grok-4.7)
Conversation: https://claude.ai/code/session_01VZjwf7Ux9YtTzwHGHMqnsq
Risk assessment (independent review, Codex gpt-6.1-sol, head 3104659)
Verdict: CHANGES
Rating: Codex gpt-6.1-sol rated it high (H1) at head 3104659
Risk: high (H1: public vector-form SCC solves reject previously working callback and return-code-carrier inputs; no major version bump accompanies these regressions).
Blast radius: Vector-form
SCCNonlinearProblem, including ordinary callable adapters around legacy FunctionWrappers and algorithms returning convertible return-code carriers. Tuple-form SCC and the default Newton/trust-region/polyalgorithm lanes are untouched. The change affects compatibility with SciMLBase ≥3.57, rather than numerical iteration formulas.Evidence: Read the linked issue and companion PR, the complete
git diff origin/HEAD...HEAD, the surrounding SCC implementation, tests, and CI results.git diff --check origin/HEAD...HEADpassed. Ran Julia 1.12.7 with released SciMLBase 3.58.0, four threads, the requested depot, and one test process at a time. Sources were archived into scratch; only the SCC source path differed between comparisons, with shared dependencies held identical. Default SCC source:origin/HEADat8d5e163e724ddfc1232055175dd8fb592915809f. The earlier companion-source comparison gave the same intended-fix/carrier results.Commands (where
RUN=<local path>,JULIA=<local path>):Observed results:
core_tests__item4.jlcore_tests__item8.jl, unchanged assertionsBoth aggregate runs exit 1 for the distinct errors above; neither is represented as a passing suite. Logs: head, default. Reproducer: targeted runner, carrier.
G1: not applicable — no numerical algorithm, tolerance, or initialization formula changes; the SCC tests compare against a separately assembled monolithic problem. G2: not applicable to the edited code — no GPU kernels, AD rules, Reactant tracing implementation, or MPI code is changed; actual end-to-end Reactant/AD compatibility remains unverified, and ordinary carrier compatibility already fails. G3: not applicable — the allocated buffer is local to a solve; no shared cache, threading, or global default is changed.
The red
benchmark (lts)check is demonstrably a default-branch failure: its log first selectsBASE_SHA=0055e314e78fcd070007ecaaeb6702116a279631, announces benchmarking that revision, then errors resolvingSciMLJacobianOperators0.1.20 while the registry offers only through 0.1.19. It never reaches benchmarking the PR revision. This is evidence from the actual base execution, not an assumption that master also fails.Merge: needs changes (fix both compatibility regressions and add discriminating tests). If rejecting those inputs is intentional, treat it as a breaking change requiring a release decision rather than calling this non-breaking.
🤖 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.