Repository navigation
ltm: core-algorithm batch of root-caused fixes from epic #488 - #761
Merged
Merged
Conversation
The synthetic aggregate node LTM hoists for a scalar-target inlined reducer over an array expression (grow = 1 + SUM(pop[*] * scale), GH #738) failed fragment compilation and was silently stubbed to a constant 0, corrupting every score routed through it. Root cause: compile_ltm_equation_fragment lowered the synthetic equation with an empty ScopeStage0.models, so Expr1->Expr2 lowering could not resolve any dependency's dimensions, the pop[*] * scale Op2 never carried ArrayBounds, and Pass-1 temp decomposition (which gates on those bounds) never hoisted the array expression out of the reducer -- codegen then rejected the inline Op2 under SUM. The fix is a shared lower_ltm_variable helper mirroring lower_var_fragment's minimal-ModelStage0 scope construction: lower once with an empty scope (byte-identical output when no dependency is arrayed, so the common scalar case pays nothing), and re-lower with self plus the deps' Stage0 variables in scope when an arrayed dependency -- a model variable or an arrayed LTM parse-time helper aux -- is present. Applied to both compile_ltm_equation_fragment and compile_ltm_implicit_var_fragment. The direct scale->grow link score still fails for a distinct reason (PREVIOUS(pop[*]) outside A2A context is captured into an ill-typed scalar helper, the documented GH #541 limitation); the new tests pin it as the only tolerated residual, and the loop score consuming it stays 0 pending GH #737's routing fix.
The GH #738 dependency-aware lowering scope regressed C-LEARN's cold LTM-enabled compile by 26% (6.00s -> 7.53s): lower_ltm_variable ran a classify_dependencies walk per fragment that duplicated the one compile_ltm_equation_fragment already performs, and the scoped re-lower (a second full lowering plus per-dep Stage0 clones and uncached parse_var calls) triggered for every fragment with any arrayed dependency -- nearly all of them on a heavily arrayed model. Two mitigations, both behavior-preserving for the #738 class. First, lower_ltm_variable now returns the dependency classification it computes (LoweredLtmVariable), and both fragment compilers consume it instead of re-walking the lowered AST; the implicit-var consumer keeps its dt-AST-only scope via an ast().is_some() guard so stock-shaped helpers keep their empty dep set. Second, a syntactic gate (equation_may_contain_reducer) skips the arrayedness analysis and the re-lower when the equation text contains no hoistable-reducer call (name-paren, case-insensitive): the recovered Expr2 bounds are consumed by Pass-1 temp decomposition, which fires only on reducer arguments, so a reducer-free fragment compiles identically either way (LTM text is always print_eqn/expr2_to_string output, which never puts a space before the paren). Gated-out fragments behave exactly as before #738 for the rarer bounds consumers too -- never worse than the pre-#738 baseline. Re-measured with interleaved release A/B (warm builds, 4 and 3 rounds): cold LTM compile on C-LEARN 5.99s (pre-#738) / 7.53s (unmitigated) / 6.31s (this commit); clearn_pinned_climate_loop_is_scored 9.25s / 10.91s / 9.65s. The ~5% residual is the per-reducer-fragment scoped re-lower -- the cost of compiling those fragments correctly -- and is quantified on GH #655 (C-LEARN per-fragment overhead). Also tightens wording around the tolerated (not required) scale->grow residual assertion and documents the lowering-scope boundary for arrayed LTM-synthetic dependencies.
The bd4376e syntactic gate skipped the dependency-aware re-lower for LTM fragments without a hoistable-reducer call in their equation text. That keyword list was derived from the wrong set: ltm_agg's agg-hoistable reducers, not ast::expr3's Pass-1 temp-decomposition sites. The two differ in both directions -- SIZE is never hoisted into an agg (its link score is constant 0) yet Pass-1 decomposes its argument exactly like SUM's, and RANK is agg-hoistable yet Pass-1 never decomposes its args. The divergence was reachable and silent: a ceteris-paribus partial wraps a non-live reducer subtree as PREVIOUS(SIZE(...)), LTM parsing captures the argument into a scalar helper whose equation is exactly SIZE(pop[*] * 2), the helper text matched no keyword, the re-lower was skipped, the bounds-gated decomposition never fired, and the helper was stubbed to a constant 0 with no diagnostic (failed implicit-helper fragments are dropped silently at assembly; tracked separately). Replace the text scan with a structural walk of the preliminary lowered AST (ast_contains_pass1_decomposition_site) whose builtin match is exhaustive with no wildcard arm and mirrors Pass-1's decomposition calls one-for-one (SUM/MEAN/STDDEV/SIZE, 1-arg MIN/MAX, the VECTOR and ALLOCATE families, and the LOOKUP family for the arrayed-GF apply). Sound by construction, and loud against future divergence: adding a BuiltinFn variant fails compilation at the gate until classified, and pass1_gate_covers_each_decomposition_builtin pins the classification of the existing variants. The SIZE regression is pinned end-to-end by size_reducer_previous_helper_compiles_and_is_correct (helper series equals the element count, link score exactly 1 past the initial step). Cold C-LEARN LTM compile is unchanged by the gate redesign (~6.28s vs the text-scan's 6.31s; pre-#738 5.99s, unmitigated 7.53s). Also corrects the stock-shaped-helper comments (Variable::ast() returns a Stock's init AST, and LTM Stage0 inputs are always Aux-parsed Vars, so the init-AST fallback was dead and is removed).
A feedback loop re-entering through a scalar feeder of a hoisted reducer (grow = 1 + SUM(pop[*] * scale), scale fed back from a scalar stock) classified PureScalar: every cycle edge's reference shape is Bare, so build_loops_from_tiered's fast path materialized the loop straight from the variable-level circuit with a direct scale->grow link. That link has no compilable score (its ceteris-paribus partial needs the lagged whole-array read SUM(PREVIOUS(pop[*]) * scale), the GH #541-class capture the engine rejects), so the loop score silently read 0. Of the two designs in the issue, this takes the classifier route: an edge with a ThroughAgg-routed reference site (a new agg_routed_edges set on EdgeShapesResult, projected from the reference-site IR) now classifies its cycle CrossElementOrMixed. One decision point covers both surfaces -- the tiered enumerator and model_pinned_loops both call classify_cycle -- and the slow path already owns every piece of agg machinery (the subgraph projection keeps agg nodes, the element-subscripted link builder handles agg endpoints, and recover_agg_hop_polarities patches the hop polarities). A fast-path splice would have duplicated that logic in build_loops_from_tiered, in the pinned PureScalar arm, and again per-slot for A2A cycles through arrayed aggs. The reroute alone is not enough: emit_source_to_agg_link_scores early-returned for a scalar from, so the rerouted loop would reference a never-emitted scale->agg name. It now emits one Bare-named score (dimensioned over result_dims for an arrayed agg, referenced per-slot by cross-element loops) whose equation uses a changed-last attribution: freeze ONLY the scalar feeder at PREVIOUS and subtract from the agg's current value. The conventional changed-first partial is inexpressible for this edge (it is exactly the uncompilable lagged-wildcard shape); changed-last is its first-order-equal dual and compiles for any reducer body since the array references stay verbatim. The direct-link residual (#541-class) remains tracked separately; these loops no longer consume it. Knock-on effects, checked: loop ids for the affected class move from u{n} to r/b{n} (polarity is now recovered via the existing agg-hop patch; its monotone source->agg arm now also covers scalar-feeder hops, a documented approximation for negating reducer bodies). Loop.stocks stay scalar names (their own element-level form), and partitions resolve unchanged through the element graph. The auto-flip gate can now see these cycles' variables in the slow-path subgraph, adding only the scalar cycle nodes plus agg nodes on top of what the variable-level gate already measured. model_detected_loops' pin dedup keys and reported variable lists are made agg-insensitive (a pin expanded through a reducer would otherwise double-surface, drop its name transfer, and leak the $-prefixed agg node); its variable-level ids can now differ from the scored ids for this class, which falls under the existing arrayed-model cross-surface caveat (reclassify_loops_from_results skips missing ids). model_pinned_loops applies recover_agg_hop_polarities to element-graph pin expansions before pin ids are assigned. The C-LEARN pinned climate loop gate (release, --ignored) was re-run green.
Adversarial review of 1dcf899 demonstrated a critical consequence of routing scalar-feeder loops through agg nodes: the scored surface (model_ltm_variables) recovered the feeder loop's polarity while model_detected_loops still derived it from the variable-level scale->grow link (Unknown), so the two surfaces' polarity-prefixed ids diverged AND collided (scored {r1: feeder, r2: pop} vs detected {u1: feeder, r1: pop}). The runtime join is keyed purely on the id -- reclassify_loops_from_results and get_relative_loop_score read $..loop_score..{id} -- so the pop-growth loop was classified from the feeder loop's series: on a negating-body variant it reported Balancing at confidence 1.0. The collision mechanism existed latently for arrayed agg-traversing models since the GH #516 polarity patch; 1dcf899 extended it to plain scalar models. Fix direction (a)+: make both surfaces derive identical polarities and ids by construction, rather than re-deriving detected loops from the tiered set (a public-surface semantics change for every cross-element model) or re-keying the join on content (which still leaves the id-based get_relative_loop_score API broken). Three pieces: - recover_agg_routed_edge_polarities (called by model_detected_loops): for an Unknown variable-level link whose reference sites are ALL ThroughAgg-routed, compose the same two hop polarities the scored surface assigns (source_to_agg_hop_polarity into the agg, agg_consumer_polarity out of it), per routed agg with agreement required. The edge's product equals the scored loop's two patched hops, so loop polarities agree by algebra. - loop_id_sort_key now excludes synthetic agg nodes from both key components: the scored loop traverses scale->$agg->grow while the detected loop carries scale->grow, and the $-prefixed agg name sorts before every letter, so keeping it would order the same cycle differently on the two surfaces. - the feeder hop itself (review I1) now gets a DISCRIMINATING polarity instead of the blanket monotone-Positive label, which was confidently wrong for negating bodies (SUM(pop[*] * (1 - scale)) labeled reinforcing with a sustained negative score): analyze_feeder_to_agg_polarity analyzes d(body)/d(feeder) with a positive-by-convention Mul one-side rule (the existing Div-arm convention, scoped to the feeder-hop analysis only and guarded by expr_references_var) -- pop[*] * scale -> Positive, pop[*] * (1 - scale) -> Negative, indeterminate bodies -> Unknown. The general analyzer is unchanged (the convention applied to arbitrary equations would relabel the logistic-growth class). Both recover passes share one source_to_agg_hop_polarity helper so the surfaces cannot drift; the arrayed reduced-source arm keeps the monotone label. Ids agree wherever the two surfaces' loop sets biject -- every scalar-cycle model, including both review fixtures, pinned end-to-end through reclassify_loops_from_results by the new cross-surface test. The per-element cross-element classes were already divergent pre-#737 (documented arrayed-model caveat; reclassify skips ids without a score series) and remain tracked separately. The C-LEARN pinned climate gate (release, --ignored) re-ran green. Also per review: the changed-last rustdoc no longer claims the changed-first feeder partial is inexpressible (a per-element frozen helper would compile; it is a cost tradeoff), the deviation is called out in docs/reference/ltm--loops-that-matter.md next to the numerator-timing convention note, and the assign_pin_ids doc reflects the content re-sort recover_agg_hop_polarities may apply before pin ids are assigned.
Round-2 review of a14a6be demonstrated that composing agg-hop polarities onto the single variable-level link (round 1's C1 fix) does not make the detected and scored surfaces bijective. A multi-agg edge (grow reading scale through TWO hoisted reducers) is two scored loops but was one detected loop, so every id after the divergence joined the wrong loop's series; and when the two aggs' hop polarities disagree the composed edge collapsed to Unknown, colliding the feeder cycle's u-id with an unrelated genuinely-Undetermined loop -- which the runtime join then classified, confidently, from the wrong series. Fix: model_detected_loops now rebuilds each enumerated loop as the cartesian product of its links' routing variants -- the direct link when the edge has any Direct reference site (a mixed Direct+ThroughAgg edge genuinely has both pathways, and the scored element graph emits both), plus one from -> $..agg..{n} -> to splice per distinct routed agg (expand_loops_through_routed_aggs). Spliced hops start Unknown and the SAME recover_agg_hop_polarities pass the scored and pinned surfaces run patches them, so polarities are identical by construction. With both surfaces' links now carrying the agg nodes, the round-1 agg-stripping in loop_id_sort_key is REVERTED: keys match exactly across surfaces, multi-agg sibling loops stay totally ordered (agg.0 vs agg.1) instead of leaning on a tied-key stable-sort fallback, and the pin-dedup rotations compare raw node sequences again (a multi-agg pin's per-agg variants each dedup onto, and donate the pin's name to, their own enumerated counterpart). The round-1 edge-composition function is deleted. Spliced agg nodes are trimmed from the user-facing DetectedLoop.variables only. Bijection boundary, precisely: for cycles whose variables are all scalar the expansion is isomorphic to the element-graph circuits the scored surface enumerates -- scalar nodes do not expand per element, an agg hoisted from a scalar consumer is itself scalar, and cross-agg petal stitching cannot fire (all petals of an agg pass through its single host variable, so no two are disjoint). That covers multi-agg and mixed Direct+ThroughAgg edges, both probe fixtures, and the original headline fixture. NOT covered, unchanged from before #737: cycles containing an arrayed variable stay variable-level here while the scored surface enumerates them per element, the id namespaces still overlap without bijection there, and the id join can still read another loop's series for that class (latent since #516; needs its own tracked fix). A defensive 64-variant-per-loop cap keeps a pathological expansion unexpanded, accepting id divergence for that model only. One FFI-visible change for arrayed mixed-edge models: a cycle like pop -> share (share = pop / SUM(pop[*])) now surfaces as two detected loops (direct and via-agg), matching the two loop families the scored surface really has. Review I1b: the arrayed source -> agg hop arm dropped its blanket monotone-Positive label, which was confidently wrong for arrayed co-source feeders: in grow = 1 + SUM(pop[*] * (1 - weight[*])) the partial d(agg)/d(weight[e]) = -pop[e] < 0, yet the detected surface reported the total -> weight -> grow loop Reinforcing at confidence 1.0 (base commit: Undetermined). source_to_agg_hop_polarity now runs the discriminating analyze_source_to_agg_polarity (renamed from the feeder-specific name) for every source: SUM(pop[*]) and SUM(pop[*] * scale) w.r.t. pop stay Positive, the co-source fixture labels Balancing on both surfaces (b-prefixed scored ids), and indeterminate bodies fall to Unknown -- never blanket-Positive. This also keeps the static label consistent with the eventually-fixed runtime weight->agg score (its series sign is wrong today; tracked separately by the reviewer). The now-dead agg_reducer_is_monotone / reducer_name_is_monotone predicates are deleted. Also fixes the docs/reference/ltm--loops-that-matter.md line-wrap artifact ("first-order -equal"). The C-LEARN pinned climate gate (release, --ignored) re-ran green.
The 2f2dbbd unification deleted the blanket monotone source-to-agg arm in favor of the discriminating analyze_source_to_agg_polarity body analysis, but this caller-side rustdoc still described the old split (monotone for arrayed rows, discriminating only for scalar feeders), contradicting the callee's updated documentation. Doc-only change, flagged as the remaining minor in the round-3 adversarial review.
project_spec_strategy never generated an inlined (sub-expression) array
reducer, so no synthetic $⁚ltm⁚agg⁚{n} node was ever minted under the
projection proptest and the ThroughAgg routing arms were verified only
vacuously -- the GH #533 fast-path bug was undetectable by construction
(GH #739). Add InlinedReducer / InlinedReducerScalarFeeder patterns for
arrayed targets plus an optional scalar 'total' reducer target, covering
both emit_agg_routed_edges arms (arrayed source rows; empty-from_dims
scalar feeder) against both target shapes, including the #533 both-scalar
shape.
The projection now splices agg nodes out (from -> agg -> to becomes
from -> to, cross product over the agg's sources x targets, fixpoint for
hypothetical agg chains) so the invariant is stated over the trimmed
graph. The projection invariant alone is insensitive to the #533 class
(a direct scale -> total edge projects to the same variable edge as the
agg hop), so both properties additionally assert spec-derived agg-hop
routing: one agg node must carry all of a reducer's source/feeder/target
hops and a pure ThroughAgg feeder must have no direct edge. Vacuity is
made structurally impossible by a second property over a strategy that
always injects one of four forced reducer shapes (asserting an agg node
exists every case) plus a deterministic companion test pinning the fixed
expected hops; re-introducing the pre-#533 fast path fails all three.
The proptest's forbidden_direct set covered only the scalar feeder, so a regression that emits correct agg hops PLUS spurious direct source-row -> target edges for an inlined reducer's ARRAYED source passed both the projection invariant (the spurious edge collapses to the legitimate variable edge) and the agg-hop existence check (subset semantics): only the swap direction (GH #533) was caught, not the additive one. Every generated inlined-reducer equation references its sources solely inside the reducer (one pattern per variable, so no spec can give the same pair a second non-reducer site), which makes ALL direct source -> target element edges illegitimate -- forbidden_direct is now the full sources x targets cross product, closing both directions. Verified by hand-broken temp worktrees: adding spurious direct edges for arrayed reducer sources in emit_agg_routed_edges now fails all three tests, and the original pre-#533 fast-path mutation still does.
cyclic_orderings' (m-1)!/2 mirror-skip rationale was mathematically wrong (GH #676): the skipped orderings reversed the petal SEQUENCE while traversing each petal forward, which is not the edge-reverse of the kept ordering -- the genuine reverse edges are not even in the element graph. Deeper, for a fixed petal subset every cyclic ordering yields the SAME edge multiset (each petal contributes its agg->head / internal / tail->agg edges regardless of position), so all orderings share one commutative-product loop_score and are indistinguishable for dominance analysis. (m-1)!/2 was consistent with neither loop identity ((m-1)!) nor distinct scores (1). stitch_cross_agg_petals -- the mode-agnostic core both the exhaustive (GH #515) and discovery (GH #696) paths feed -- now emits ONE canonical loop per pairwise-disjoint petal subset: the chosen petals concatenated in the deterministic priority order, so per-run determinism (and assign_loop_ids stability) is preserved. cyclic_orderings and heaps_permutations are deleted. The recoverable count per agg drops from super-exponential to 2^k - k - 1, so the MAX_CROSS_AGG_LOOPS = 256 budget no longer burns slots on score-identical duplicates and genuinely-distinct subsets truncate later. Loop ids renumber for multi-petal models (content-derived, expected).
model_pinned_loops rejected any pin whose cycle had no PARENT-level
stock, checking the raw cycle against model_causal_edges.stocks. A
DynamicModule node (a SMOOTH/DELAY instance or stock-carrying user
sub-model) is never in that set, so a valid feedback loop whose only
state lives inside a module it traverses -- e.g.
driver -> module -> reader -> driver -- was wrongly rejected as
'contains no stock', even though the loop enumerator finds and scores
the identical cycle (it applies no stock filter and attaches
module-internal stocks via enrich_with_module_stocks).
The validation now applies the enumerator's exact stock semantics:
enrich the cycle's parent-level stocks with module-internal stocks
(enrich_with_module_stocks, widened pub(super) -> pub(crate)) and
reject only when the enriched set is empty. That keeps the intended
rejection for a purely-instantaneous cycle: a stockless passthrough
module enriches to nothing, so a no-stock-anywhere pin still fails
with the clear diagnostic (such a cycle is a compile-time circular
dependency, not a feedback loop).
Root-cause notes: a pin can only name datamodel variables (LoopMetadata
UIDs), so the module-bearing cycle reaching this check is an explicit
Module variable; a SMTH1-builtin loop's cycle traverses the synthetic
$⁚{var}⁚{n}⁚smth1 node, which a pin cannot name -- that pin fails
earlier at order_variable_cycle and is a separate, pre-existing gap.
Three review minors from the GH #673 adversarial review: the purely-instantaneous-cycle rationale was false for a stockless cycle broken by PREVIOUS (it compiles and the enumerator scores it -- the surfaces-disagree gap is now tracked as GH #749); the enrichment attribution named build_loop_from_cycle as the enumerator's path when the detection surface actually enriches via find_loops_with_limit; and a test-helper doc sentence was truncated mid-thought.
emit_agg_to_target_link_scores pinned an arrayed synthetic agg's references in the per-target-element equation BODY via deps_to_subscript, i.e. to the target's full element tuple. That is correct only for the diagonal case (result_dims == to's dims); in the strict-prefix broadcast case (agg over [D1] feeding to[D1,D2], e.g. SUM(matrix[D1,*]) inside an A2A body over D1 x D2) the full tuple over-subscripts the 1-D agg, the fragment fails to compile, and the score (and every loop score through the agg) is stubbed to a constant 0 with an Assembly Warning. The link-score NAME and the Dsource denominator already projected the target element onto result_dims' axes; the body now does too, via a new source_pin_element parameter on generate_scalar_to_element_equation (a second subscript_idents_at_element pass over just the source ident, mirroring the existing source_ref_override denominator mechanism). Only the broadcast case changes: for the diagonal case the projection IS the full tuple, verified byte-identical against the parent commit for the diagonal arrayed-agg, scalar-to-arrayed, and sliced-scalar-agg emission paths. build_model_with_failing_ltm_fragment still fails as the diagnostic-infrastructure tests require, but for a distinct (still-open) reason -- variable-backed partial-reduce loop scores referencing link-score names that are never emitted -- so its doc comment now describes that failure instead of GH #528.
expand_same_element matched source and target dimensions by name only, so a Bare edge between differently-named but MAPPED dimensions (x over Region feeding target[State] with a State->Region mapping, the AC3.5 case classify_iterated_dim_shape already recognizes as Bare) fell into the broadcast branch and emitted the full Region x State cross-product instead of the mapping's diagonal (GH #527). The element correspondence now lives in one reusable place, DimensionsContext::mapped_element_correspondence(iterated_dim, source_dim): per iterated element, the source element the compiler's own translate_via_mapping resolution reads. Direction: both declaration directions are honored, mirroring the compiler -- the iterated-dim- declared direction is the one the classifier's mapped-Bare arm accepts (has_mapping_to(index_dim, source_dim)); the source-declared direction only arises for bare (unsubscripted) references, which classify Bare without consulting mappings (a subscripted reverse-declared reference stays DynamicIndex and keeps the conservative cross-product, so classification and expansion never disagree: classification says Bare implies the expansion derives exactly one source element per target element). Transitivity: single-hop only, matching has_mapping_to. Cardinality: an explicit element map may be many-to-one (State{s1,s2,s3}->Region{a,b} yields 3 edges, one per TARGET element); a partial/malformed map or positional size mismatch returns None and callers keep the conservative broadcast. expand_same_element inverts the correspondence per source element (preimage), composing with the existing partial-collapse and target-only-dim broadcast machinery. link_score_dimensions gets the same correspondence in its compatibility rule: the mapped Bare A2A edge previously got a SCALAR link score whose equation referenced arrayed variables in scalar context -- a fragment compile failure silently stubbed to constant 0 -- while loop-score equations subscript the Bare name per slot. With the target's dims the equation compiles (its references resolve through the same mapping the model's own equations use) and the per-slot references resolve, so a mapped-dim feedback loop now scores finite, sustained non-zero in exhaustive mode (previously all-zero, with 4 of 6 enumerated loops spurious cross-products). Discovery mode still cannot score mapped-dim loops: expand_a2a_link_offsets subscripts both sides of an A2A score with the same element, minting phantom from-side keys for a mapped edge -- tracked separately. The agg emitters' mapped-axis carve-out is GH #534, which will reuse mapped_element_correspondence.
Review of a010931 demonstrated that for EXPLICIT (non-positional) element maps the diagonal followed the element map while the engine's executed A2A lowering resolves mapped references POSITIONALLY, ignoring the map -- so the element graph dropped the true positionally-read edges (an under-approximation; loops through such an edge would silently vanish, where the pre-#527 cross-product at least contained them). The translate_via_mapping helpers honor element maps, but the lowering path actually executed for mapped A2A references never consults them; different-cardinality maps fail to compile outright (GH #753), consistent with positional-only execution. That engine inconsistency is pre-existing and tracked separately. mapped_element_correspondence now returns None whenever a non-empty element_map is declared between the pair (either direction), keeping the diagonal ONLY for positional mappings; None means callers keep the conservative broadcast, a superset of the simulation's reads. The alternative -- returning the positional correspondence even when an element map is present, matching execution today -- would knowingly encode the engine bug into LTM results and silently turn into a wrong-map diagonal the day execution honors the maps, so the conservative None is preferred; the rustdoc names the engine fix as the gate for re-enabling element-map diagonals, and the emitter keeps the general preimage inversion so re-enabling needs no emitter change. The truthful invariant (softened in the surviving docs): a Bare classification yields the mapping diagonal WHEN a usable correspondence exists, else the conservative broadcast -- never fewer edges than the simulation reads. A new graph-vs-simulation parity test on an asymmetric permuted element map derives the implied edges from the run's actual values and asserts the graph is a superset, so it keeps passing if the engine later honors element maps and the diagonal returns. The 3->2 element-map test flips to pinning the broadcast (the model cannot compile per #753, so the graph-level pin is all there is); all positional fixtures, including the end-to-end mapped feedback-loop scores, are unaffected.
compute_read_slice declined a sliced reducer whose iterated index only lines up with the source's row axis via a dimension mapping (SUM(matrix[State,*]) over matrix[Region,D2] with a State->Region mapping), so the reference fell back to the conservative full cross-product (GH #534). AxisRead::Iterated now carries the (target, source) canonical dim pair -- equal for the literal case -- and a positionally-mapped axis is accepted when the mapping is declared on the iterated dim toward the source's dim (the same direction classify_iterated_dim_shape's mapped arm accepts; the reverse-declared direction is GH #757 and stays conservative) AND iterated_axis_slot_elements yields a usable slot remap. iterated_axis_slot_elements (ltm_agg.rs) is the single slot-remap decider all three Iterated-axis consumers share: per source element of the axis, the agg result-slot coordinate is the TARGET element whose mapped_element_correspondence entry names that source element -- the PREIMAGE inversion of the GH #527 helper, written for the general many-to-one shape but bijective today because the helper's positional-only gate (explicit element maps decline, GH #756) is inherited for free. result_dims carry the TARGET dim, so the agg aux is ApplyToAll over the target's iterated dim (the engine's own positional mapping resolution compiles the reducer text unchanged) and the GH #528 agg-to-target projection composes with no change. The emitters -- emit_agg_routed_edges (element graph) and read_slice_rows behind emit_source_to_agg_link_scores (link scores) -- apply the remap between row enumeration and slot naming; a missing remap degrades to the same conservative fallback as a malformed read_slice rather than emitting mis-slotted edges, though it is unreachable for a slice compute_read_slice accepted (both gate on the same helper over the same salsa dimension context). One deliberate exception to the variable-is-the-agg rule: a whole-RHS mapped reducer (out[State] = SUM(matrix[State,*])) mints a SYNTHETIC agg instead of a variable-backed one. The variable-backed link-score path (try_cross_dimensional_link_scores' partial-reduce arm) matches result axes against source axes by name, so a remapped pair falls off it onto the per-shape Wildcard partial, whose PREVIOUS-wrapping mangles the iterated index into the non-compiling matrix[PREVIOUS(state),*] -- a silently-stubbed constant-0 score (and 8 spurious cross-product loops where the synthetic route scores the 4 real ones). The un-mapped literal case is byte-identical: the emitted LTM vars (names, equations, dims, compile_directly), loop_partitions, and element edges for six literal fixtures (arrayed sliced diagonal + broadcast, pinned slice, whole-extent, whole-RHS partial reduce, scalar co-feeder) were dumped at the parent commit and after this change in separate worktrees and diffed clean.
The 74f5366 carve-out docs said a coarse conservative score is the right semantics for the declined mapped-reducer cases, but the review demonstrated the emitted scalar score references A2A idents and fails fragment compilation there, stubbing loop scores to 0 (now tracked as GH #758) -- the cross-product is sound only for the element EDGES. Also notes the mapped-reducer reason resolve_result_dim consults to_dims, alongside the scalar-feeder one.
GH #679 asked for runtime polarity reclassification of exhaustive-mode loops so the Mostly* (Rux/Bux) labels stop being structurally unreachable. Recon against the current branch shows the production gap is already closed by the #737/C1 work: reclassify_loops_from_results classifies each detected loop's full loop_score slot set via LoopPolarity::from_runtime_scores (real dominance-ratio confidence), and simlin_analyze_get_loops_runtime applies it on the libsimlin C ABI with all five polarity variants surfaced verbatim (#495). What was missing is any proof that a definite Mostly* outcome is reachable end-to-end: existing tests pin only the U->R single-sign flip, the keep-structural fallback, and a sign-straddling case loose enough to accept Undetermined. Close that residual with a canonical mixed-sign fixture (f = s*g + d: marginal gain g flips +0.02 -> -1e-4 at t=100 while an exogenous ramp d dominates delta-f, so ~400 loop-score samples of +1 precede 40 negative samples of ~1e-4..1e-3 magnitude; dominance ratio ~0.9999). New tests pin: the engine reclassifier yields MostlyReinforcing with confidence in [0.99, 1.0) where the structural label was Undetermined/0.0; a comparable-magnitude variant stays Undetermined with a real non-zero sub-threshold confidence; discovery mode reports the identical Rux label and confidence (to 1e-6) on the same fixture; and the C-ABI runtime surface reports SimlinLoopPolarity::MostlyReinforcing with the real confidence and a stable loop id. Tests only -- no production code change was needed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #761 +/- ##
==========================================
+ Coverage 90.65% 90.76% +0.10%
==========================================
Files 224 224
Lines 136448 137694 +1246
==========================================
+ Hits 123697 124978 +1281
+ Misses 12751 12716 -35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fca4cfeca7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+122
to
+125
| td.name() == fd.name() | ||
| || dim_ctx | ||
| .mapped_element_correspondence(td.canonical_name(), fd.canonical_name()) | ||
| .is_some() |
There was a problem hiding this comment.
When the only mapping is declared in the reverse direction, classify_iterated_dim_shape still declines the subscripted reference because it requires has_mapping_to(index_dim, source_dim), so the element graph treats source[State] over source[Region] with Region -> State as a conservative DynamicIndex cross-product. This new compatibility check calls mapped_element_correspondence(State, Region), which also succeeds for the reverse-declared mapping, so the DynamicIndex link score is retargeted to State and reused for every conservative off-diagonal edge into the same target slot. In that scenario false-positive cross edges can now get the real diagonal score instead of staying unscoreable/conservative, producing spurious nonzero loop scores; this check should match the classifier’s accepted mapping direction or be gated by the actual reference shape.
Useful? React with 👍 / 👎.
Code reviewReviewed the core algorithmic changes (analysis.rs, dimensions.rs, ltm/compile.rs, ltm/loops.rs, ltm/link_scores.rs, ltm/pinned.rs, ltm_agg.rs, ltm_augment.rs, ltm/polarity.rs, ltm/graph.rs) across three parallel passes focused on:
Edge cases checked: empty dims, duplicate dim names, missing Mapped preimages, both-direction map declarations, element-map vs positional gates, id re-assignment ordering across No correctness bugs found. The changes are internally consistent and the documented invariants (POSITIONAL restriction, exhaustive-decomposition gate, canonical-petal-subset deduplication, changed-last vs changed-first complementary attribution) hold up under inspection. Overall correctness verdict: correct. 🤖 Generated with Claude Code |
PR #761 review (r3389029131) found that link_score_dimensions' mapped-correspondence arm fired for BOTH mapping declaration directions (mapped_element_correspondence deliberately accepts both, since bare references resolve through either), while classify_iterated_dim_shape accepts a SUBSCRIPTED mapped reference only in the forward direction: a reverse-declared subscripted reference classifies DynamicIndex (GH #757) and the element graph emits the conservative cross-product. Retargeting that edge's (Bare-named, since Wildcard/DynamicIndex collapse onto the Bare name) link score to ApplyToAll over the target's dim shapes per-slot DIAGONAL partials that the off-diagonal loop links then read by target-element subscript -- wrong-slot values where the pre-#527 behavior was the conservative scalar stub (GH #758). The fix aligns classification with emission: the mapped arm now requires the (from, to) edge to have a Bare-classified reference site (consulting model_edge_shapes, the same projection the cycle classifier reads), which is exactly the condition under which expand_same_element emits the mapped diagonal -- so "link score arrayed over target dims" holds iff "element edges are the diagonal". The same-name compatibility arms stay shape-independent, and forward- declared Bare edges (subscripted or bare) keep the arrayed score. A mixed edge (Bare AND DynamicIndex sites on one pair) keeps the arrayed score the Bare site needs while its cross-product links still read diagonal slots -- the pre-existing mixed-shape conservatism family, documented at the gate, not changed here. Empirically, in the minimal pure reverse-declared fixture the mis-dimensioned score's DynamicIndex-shaped equation happened not to compile (stubbed to 0 with a Warning), masking the wrong-slot reads; the hazard was one equation-builder improvement away from surfacing, and the dims disagreed with the element graph's actual expansion either way. Post-fix the fixture lands in the explicit conservative regime: scalar score, stub warnings, all loop scores 0.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Reviewer reportI split the diff across three reviewers covering After the sweep I have no findings to flag. Spots I scrutinised most carefully and verified sound:
One latent item worth noting (NOT a regression — pre-existing): Overall verdictCorrect. The patch is internally consistent, the GH #527/#534/#528/#676/#673/#679 fixes match their documented invariants, and conservative fallbacks are preserved everywhere |
This was referenced Jun 10, 2026
bpowers
added a commit
that referenced
this pull request
Jun 11, 2026
Second burn-down batch on epic #488, following the PR #761 pattern: fourteen issues fixed across 28 commits, each implemented by a fresh agent under TDD and adversarially reviewed by a separate fresh agent to zero material findings (most took 2 rounds; the reviews caught a demonstrated wrong-score regression, two falsified soundness claims, a live layout misjoin, and four stale public contracts before merge). The batch invariant throughout: no silent wrong numbers -- every un-scoreable shape now either scores correctly or degrades to one loud warning, never garbage and never a quiet zero. ## What changed, by cluster **Polarity recovery (#745).** Arrayed-agg loop hops now match their subscripted element-graph endpoints, so loops through arrayed reducers classify concretely (r/b) instead of blanket-Undetermined, shrinking the #746 cross-surface polarity disagreement. **Body-aware reducer partials (#744, #762).** The per-row source-to-agg link score is now derived from the reducer BODY (live row, frozen co-reduced rows and co-sources) instead of asserting a bare unit coefficient: `SUM(pop[*] * (1 - weight[*]))` w.r.t. weight now scores negative as the true partial demands (it read sustained-positive before), with the changed-first/changed-last bilinear additivity contract preserved exactly and pinned. The MIN/MAX/STDDEV arm got the same treatment (a coefficient inside MIN previously produced garbage scores around 2884); literal self-references bail to the delta-ratio fallback rather than emitting a cancellation-violating partial. The TIME-bearing-body anchor caveat is documented and tracked (#763). **Conservative-path scoring (#752, #743, #758, plus the #510 unification).** Whole-RHS partial reducers get read-slice element edges through their own variable nodes, so their loops compose real per-(row,slot) link scores instead of referencing names that were never emitted (silent zeros before). Un-hoisted multi-source reducers' feeder edges now render a compiling changed-last partial (a constant -250-class garbage score before); when neither attribution convention can render, the edge skips loudly -- one warning naming it, no link-score variable, and loop scores through it dropped rather than zero-stubbed (the same drop now also covers the #510 disjoint-dim skip, fixing a latent double-warn). Dim-incompatible conservative scores (the element-mapped #756/#757 declines) take the same loud-skip treatment: one warning instead of 17-22 cascading fragment failures. Pinned-mixed slices are excluded from the new read-slice gate until the underlying divisor bug (#765) is fixed, keeping them loud rather than silently wrong. **Visibility (#741, #748).** Failed LTM implicit-helper fragment compiles now emit a Warning naming the helper and its parent score variable -- this silent-stub mechanism turned out to be the hidden leg of four separate bugs in this batch alone. The stock-free early return is module-state-aware (transitively, including SMOOTH/DELAY builtin instances), so a root model whose only state lives inside modules runs the LTM pass instead of silently emitting nothing; both query surfaces consume one shared stateless predicate and the bail reads its mode from the shared query, making gate drift structurally impossible. **PREVIOUS capture/wrap class (#759, #742).** Dimension-name subscript indices are no longer PREVIOUS-wrapped (both generator-side dep filtering and a wrapper-side guard), fixing reachable silent wrong scores on mundane shapes like `growth[D1] = matrix[D1, c1] * frac[D1]` -- and, it turned out, the live leg of #525's filed compile failure. Frozen array-valued RANK subtrees capture into arrayed helpers (the #541 mechanism extended), so `PREVIOUS(RANK(...))` compiles per element instead of corrupting link scores through ill-typed scalar helpers. **#525 is deliberately NOT closed by this PR.** Its filed hard failure is fixed (routed through #759), and the genuine reference now scores exactly +1 -- but the iterated+literal classification residual remains: phantom cross-element loops from the conservative cross-product now carry confident nonzero scores (~0.245 in the repro), pinned loudly in tests with a flip note, and #525 has been rescoped/retitled to track exactly that. **Cross-surface identity (#746).** `model_detected_loops` now builds its loop set with the scored surface's exact machinery (tiered circuits, shared budget, shared polarity recovery), deleting the #737-era per-agg splice and its silent 64-variant cap entirely: detected and scored ids, polarities, and partitions agree by construction for every model shape, the runtime id join can no longer read another loop's series on arrayed models, and a previously-unknown live misjoin in diagram layout's loop-importance pairing is fixed as a consequence. Exhaustive partitions move to element granularity, so partition stock sets -- the durable cross-surface key -- agree across surfaces for arrayed models too. **Pins and normalization (#749, #750).** Stockless PREVIOUS-lagged cycles are accepted as genuine feedback on every surface (the LTM reference lists PREVIOUS among state-retaining builtins, and the measured scores are meaningful), including a parent-level lag of a module output; pin validation and the stateless gate agree by construction. Unpartitioned (None-partition) loops normalize alone instead of sharing one default bucket -- the cross-pollution was live on four surfaces, including a discovery-mode case where an unrelated large loop could censor a small module-internal loop out of the result set entirely. ## Discovered along the way Twelve previously-untracked pre-existing defects were filed and added to epic #488 (#763 through #774, notably #765's pinned-slice divisor bug and #767's feeder-sub-slice hoist), #759 and #525 were rescoped with corrected severity and reachability, and scoped follow-up notes were added to #655, #683, and #716. Fixes #741 Fixes #742 Fixes #743 Fixes #744 Fixes #745 Fixes #746 Fixes #748 Fixes #749 Fixes #750 Fixes #752 Fixes #758 Fixes #759 Fixes #762
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
1 participant
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.
This batch works through the nine bug/correctness items in epic #488's Core algorithm group (the two capability features there, #658 and #674, are deferred to a follow-up branch). Each issue was implemented by a fresh agent under TDD and then adversarially reviewed by a separate fresh agent, iterating until zero material findings; several fixes took 2-3 review rounds, and the reviews repeatedly caught real defects before merge (a compile-time regression, an unsound lowering gate, two loop-id-collision mechanisms, and a polarity-soundness hole among them).
What changed, by cluster
Scalar-target agg routing (#738, #737, #739). LTM synthetic fragments now lower with a minimal model scope so arrayed deps carry dimension bounds -- the hoisted
SUM(pop[*] * scale)agg for a scalar target compiles instead of silently stubbing to 0. The lowering re-pass is gated by a structural walk of Pass-1's actual decomposition node set (an exhaustive match, so a new builtin fails compilation until classified); cold C-LEARN LTM compile cost is +5% (was +26% before mitigation; quantified on #655). Cycles traversing a ThroughAgg edge are classified off the PureScalar fast path so their loop scores compose the agg-half link scores -- the scalar feeder half uses a changed-last attribution (exactly complementary to the per-row changed-first shortcut, so additivity is exact for bilinear bodies; documented as a deviation in the reference doc). The detected surface now splices one loop variant per routed agg so detected/scored loop ids biject for scalar-cycle models (the id join previously read the wrong loop's series); a discriminating body-polarity analysis replaced the blanket monotone source-to-agg label, which was confidently wrong for negating and co-source bodies. The element-graph projection proptest now force-generates inlined reducers and asserts agg-hop routing (the projection invariant alone is provably blind to this bug class), mutation-tested in both directions.Cross-agg enumeration (#676).
cyclic_orderings' (m-1)!/2 mirror-skip rationale was mathematically wrong (the skipped orderings were not edge-reverses); since every cyclic ordering of a fixed petal subset yields the same edge multiset and hence the same score, recovery now emits one canonical loop per disjoint petal subset, freeing the 256-loop budget for genuinely distinct structure. Counts on >=4-petal models drop (13 -> 11 in the four-petal fixture); both exhaustive and discovery share the change via the common stitcher.Pins (#673). Pin stock-validation now uses the enumeration surface's exact semantics (
find_stocks_in_loop+enrich_with_module_stocks), so a loop whose only stock lives inside a SMOOTH/DELAY or user sub-model validates instead of being rejected "contains no stock"; stockless passthrough cycles are still rejected. First test coverage for pins through modules.Arrayed agg shapes (#528). The broadcast agg-to-target link-score equation body now pins the agg ident to the target element's projection onto
result_dims(mirroring the already-correct name and denominator projections), soagg[D1]feedingto[D1,D2]compiles and scores instead of stubbing every loop through it to 0. The diagonal case is byte-identical.Mapped dimensions (#527, #534). Same-element edges across a mapped dimension pair project along the mapping diagonal instead of the full cross-product, and mapped-dimension sliced reducers hoist into aggregate nodes (the Iterated axis now carries a (target dim, source dim) pair remapped via one shared correspondence helper). Both are deliberately restricted to POSITIONAL mappings: review demonstrated the engine's executed A2A lowering ignores explicit element maps and resolves positionally (filed as #756), so element-map cases stay conservatively broadcast/un-hoisted until that is resolved -- a graph-vs-simulation parity test pins the boundary and survives the future engine fix. Mapped whole-RHS reducers mint a synthetic agg (documented exception) because the variable-backed Wildcard partial mangles the iterated index (#759); this turns 8 spurious zero-scored loops into 4 exactly-scored ones.
Runtime polarity (#679). The issue's ask (post-sim Rux/Bux reclassification on the exhaustive surface via
from_runtime_scores) turned out to be satisfied byreclassify_loops_from_results+simlin_analyze_get_loops_runtimein the branch's ancestry; what was missing was any proof a definite MostlyReinforcing was reachable. This lands mixed-sign fixtures pinning Rux at confidence 0.9999 (and a sub-gate Undetermined control) end-to-end through the engine and the libsimlin C-ABI, with discovery/exhaustive parity bit-identical.Discovered along the way
The adversarial reviews surfaced twenty previously-untracked pre-existing defects, all filed and added to epic #488: #741-#752 and #753-#760 (notably #756, silent wrong simulation results for explicit element maps, and #746, the arrayed-cycle loop-id namespace overlap).
Fixes #738
Fixes #737
Fixes #739
Fixes #676
Fixes #673
Fixes #528
Fixes #527
Fixes #534
Fixes #679