RFC: priority-aware + canonical tie-breaks in tearing (order-robust tear-variable selection)#96
Conversation
AayushSabharwal
left a comment
AayushSabharwal
left a comment
There was a problem hiding this comment.
This looks pretty good overall. I'm a little concerned about the canonical_ranks building. Calling string on a variable/expression is bad and I want to avoid it as much as possible (including finding a way around the lexicographic sorting of equations). It may be possible to replace the strings with a Vector{Union{Symbol, Int}} with some amount of work.
I also would like to avoid merging this until the effect on TTFX is measured, given the env-var based dump (despite its usefulness) and additional string-ification.
| # semantics of dummy-derivative state selection. The sort is | ||
| # stable, so behavior is unchanged whenever priorities are equal | ||
| # (e.g. all default 0). Do not mutate the graph's adjacency list. | ||
| vs = sort(Int[v for v in vs]; by = varpriority, alg = Base.Sort.DEFAULT_STABLE) |
There was a problem hiding this comment.
| vs = sort(Int[v for v in vs]; by = varpriority, alg = Base.Sort.DEFAULT_STABLE) | |
| vs = copy(vs) | |
| sort!(vs; by = varpriority, alg = Base.Sort.DEFAULT_STABLE) |
There was a problem hiding this comment.
Applied (with sort! on the copy), thanks.
|
Addressed the review in 9bed425: No more stringification
canonical_sort_key(v) :: Tuple{Symbol, NTuple{N,Int}, NTuple{M,Int}}
# name indices opsig
built with the existing One clarification on "the lexicographic sorting of equations": the PR never sorts equations — only this once-per- TTFXMeasured on the MultibodyComponents.jl test env (Julia 1.12.6, same machine, full
All deltas are below run-to-run spread — no measurable TTFX impact. (The ENV-gated dump compiles to a single |
|
Hmm, what about runtime after TTFX on a large-ish model? i.e. something with a few thousand variables? |
|
Runtime-after-TTFX on a large-ish model, as requested: the quadrotor (
All timing deltas — first and warm — are within single-run noise; warm For completeness, the same measurement without #97 (merge-base + this PR) is also indistinguishable: first 21.49 / 28.45 / 17.56 s, second 1.87 / 2.49 / 2.48 s, identical structure and trajectory to the with-#97 + this-PR column. |
|
CI failures addressed in ec4b57e:
|
| opsig = () | ||
| x = v | ||
| while true | ||
| stripped = @match x begin | ||
| BSImpl.Term(; f, args) && if f isa Differential end => begin | ||
| opsig = (opsig..., 1) | ||
| args[1] | ||
| end | ||
| BSImpl.Term(; f, args) && if f isa MTKBase.Shift end => begin | ||
| opsig = (opsig..., 2, f.steps) | ||
| args[1] | ||
| end | ||
| BSImpl.Term(; f, args) && if f isa SU.Operator && length(args) == 1 end => begin | ||
| opsig = (opsig..., 3) | ||
| args[1] | ||
| end | ||
| _ => nothing | ||
| end | ||
| stripped === nothing && break | ||
| x = stripped | ||
| end |
There was a problem hiding this comment.
This opsig approach is really bad for type-inference - the type after the while true is completely uninferrable. Can this be a Vector{Int} instead?
There was a problem hiding this comment.
Done in e496df0 — opsig and idxs are now built as Vector{Int} (via push!) instead of growing tuples, so the key is a concrete Tuple{Symbol, Vector{Int}, Vector{Int}} and inferrable. Vectors compare lexicographically, so the sort order is unchanged. (Fixed idxs too since it had the same growing-tuple pattern.)
- CarpanzanoTearing heuristics 1&2 consult structure.state_priorities as tie-breaks (high priority preferred as tear variable); behavior-identical on uniform priorities - tear_graph_modia candidate ordering by priority (inert in default pipeline) - MTKT_DUMP_TORN env-gated torn-set dump in generate_system_equations! Context: SciML/ModelingToolkit.jl#4612 — insufficient alone (array-component and connection-set-alias ties are metadata-indistinguishable); canonical tie-breaks needed for order-determinism.
…ore parent array)
- SystemStructure gains canonical_ranks (lexicographic rank of variable names), built in TearingState alongside state_priorities - CarpanzanoTearing heuristics break remaining ties by (priority DESC, canonical rank ASC), making tear-variable selection deterministic under semantically-neutral equation/declaration reordering (MTK#4612)
…y+stable sort!
- canonical_ranks now sort by (name::Symbol, indices::NTuple{Int}, opsig::NTuple{Int})
built structurally from the variable (getindex indices; operator chain with
Differential/Shift encoding) — no string() on symbolic expressions.
- tearEquations!: apply suggested copy + in-place stable sort!.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fullvars can contain compound expressions (e.g. multi-argument clock operators over non-variable arguments), for which getname throws 'matching non-exhaustive'. Introduce canonical_name, total over all BSImpl variants: named variables key off their name, operator/function applications off their operation's name, remaining structural variants off fixed sentinels. Key collisions are acceptable — the canonical tie-break then falls back to the original order among colliding entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per review: `opsig` (and `idxs`) were accumulated as `(opsig..., …)` inside
the loop, so their type — and hence the key type after the loop — is
uninferrable. Build them as `Vector{Int}` instead. Vectors compare
lexicographically, so the sort order is unchanged, and the returned key is
now a concrete `Tuple{Symbol, Vector{Int}, Vector{Int}}`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AayushSabharwal
force-pushed
the
priority-tearing-prototype
branch
from
e496df0 to
ec3cc20
Compare
2 participants
Motivation
SciML/ModelingToolkit.jl#4612: algebraic tearing is equation-order sensitive — reordering four semantically-identical
connectstatements in a multibody model flips it from reliably-solvable toInitialFailureat every initialization seed. Users currently have no way to influence the tearing result;state_priorityis only consulted by dummy-derivative state selection.What this adds (prototype, draft)
CarpanzanoTearing(the default algorithm viaDummyDerivativeTearing{CarpanzanoTearing}): heuristics 1 & 2 prefer keeping HIGH-state_priorityvariables as tear (iteration) variables, mirroring the dummy-derivative semantics. Model libraries can express e.g. "cut forces/torques are poor tear variables" by a negative priority on connector flows.SystemStructuregainscanonical_ranks(lexicographic rank of variable names, built alongsidestate_prioritiesinTearingState); remaining ties resolve by(priority DESC, canonical_rank ASC)— deterministic under equation/declaration reordering.build_state_priorities(scalar variable before parent array) — currently unreachable from the user API becauseAtomicArrayDictrejects indexed keys (see below), included for forward-compatibility.tear_graph_modia, and an ENV-gated torn-set dump (MTKT_DUMP_TORN) in reassembly that was essential for diagnosing all of this — can be dropped from the final PR if unwanted.All changes are behavior-identical when priorities are uniform and names are not consulted (strict comparisons keep first-found candidates).
Validation (multibody quarter-car from MTK#4612, both connect orderings)
body_upright.frame_b.f[3]/b1.frame_a.tau[3]f,tau= −2)r2.frame_b.tau[3]/r2.frame_b.tau[3]— convergedKnown gaps (intentionally out of scope here, for discussion)
v_0[1]vsv_0[3]) that is decided before the Carpanzano heuristics — in the initialmaximal_matching/ single-solvable-equation cascade, which iterate in equation order. Full determinism additionally needs canonical ordering at those stages (equation-level canonicalization is harder: equations have no stable identity under reordering).v_0[1] => 5) are rejected byAtomicArrayDict("treats symbolic arrays as atomic") — the user-facing representation needs extending before item 3 is reachable.cc SciML/ModelingToolkit.jl#4612, #95