Skip to content

fix: record array elements in collect_operator_variables - #4982

Draft
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:fix-collect-operator-variables-arrays
Draft

fix: record array elements in collect_operator_variables#4982
ChrisRackauckas-Claude wants to merge 1 commit into
SciML:masterfrom
ChrisRackauckas-Claude:fix-collect-operator-variables-arrays

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

Note

Draft — please ignore until reviewed by @ChrisRackauckas.

The bug

collect_operator_variables records arguments(v)[1] for each operator application. When
the operator is applied to an array variable or a slice — D(u[2:4]) — that argument is
the array, not its elements. Every caller tests membership of the scalar unknowns, so
nothing ever matches.

The visible consequence is in DAEProblem, which builds

diffvars = collect_differential_variables(sys)
differential_vars = map(Base.Fix2(in, diffvars), unknowns(sys))

so a system whose equations differentiate an array slice reports every variable as
algebraic
, and DAE initialization then has nothing to work from.

Failing before, passing after

@variables w(t)[1:4]
collect_differential_variables(D(w[2:3]) ~ w[1:2])
result membership map over unknowns
before Set([w[2:3]]) [0, 0, 0, 0]
after Set([w[2], w[3]]) [0, 1, 1, 0]

Measured downstream on a real system (a MethodOfLines finite-difference discretization
that emits its interior as one array equation), DAEProblem's differential_vars goes
from 0/21 to 19/21 at one grid size and 0/81 to 79/81 at another — 19 and 79 being
the interior points, with the two boundary points genuinely algebraic.

The change

One function. When the operator's argument is array-valued, scalarize it and record the
elements; otherwise behave exactly as before, so scalar operators take an identical path.

Tests

lib/ModelingToolkitBase/test/variable_utils.jl gains a testset covering a whole array
variable, a slice, an unaffected scalar derivative, and the differential_vars-style
membership test that motivated the fix. It fails on unpatched code with the
[0, 0, 0, 0] map shown above.

Test Summary:  | Pass  Total      Time
variable_utils |  145    145  11m03.2s

(145 = 141 pre-existing + 4 added.)

Not verified

I ran the variable_utils file, not the full ModelingToolkitBase suite — the change is
additive and confined to one branch of one function, but a full run has not been done
locally. Other callers of collect_operator_variables (collect_applied_operators,
Shift-based discrete paths) are unaffected for scalar arguments and now see elements
rather than arrays for array ones, which is the intended correction but is worth a
reviewer's eye.

Context

Found while making finite-difference PDE discretizations emit array (slice-form) equations
instead of one scalar equation per grid point — see SciML/MethodOfLines.jl#428. This is one
of several places where array equations lose their shape; the others (equation counting,
implicit-DAE codegen row indexing) are more invasive and will be proposed separately.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S1dxUEPGysx27SSSpxtiQ9

An operator applied to an array variable or a slice, such as `D(u[2:4])`, names the array
rather than its elements. Every caller tests membership of the scalar unknowns, so nothing
matched and the variables were silently misclassified.

The visible consequence is in DAEProblem: `differential_vars` is built as
`map(Base.Fix2(in, collect_differential_variables(sys)), unknowns(sys))`, so a system whose
equations differentiate an array slice reports *every* variable as algebraic.

Before:  collect_differential_variables(D(w[2:3]) ~ w[1:2]) == Set([w[2:3]])
         membership map over unknowns == [0, 0, 0, 0]
After:   Set([w[2], w[3]])
         membership map over unknowns == [0, 1, 1, 0]

Scalar operators take the identical path as before.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S1dxUEPGysx27SSSpxtiQ9

@AayushSabharwal AayushSabharwal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments in #4982 on this part of the code apply here.

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.

3 participants