engine: close ltm phase 1 residuals#795
Merged
Merged
Conversation
When `link_score_equation_text_shaped` could not render a compilable ceteris-paribus partial it warned and skipped the link-score variable but collapsed the failure into a bare `None`, indistinguishable from a benign "no variable here". The emission loop therefore never recorded the edge in `unscoreable_edges`, so loop scores traversing it kept referencing the never-emitted link-score name, failed fragment compile, and degraded to a cascade of warned constant-0 stubs -- the exact "warned wrong number" shape the GH #758 loud-skip contract exists to prevent. The query now returns a three-state `ShapedLinkScore` (`Scored`/`Unscoreable`/`NoVariable`) so the emission loop can tell a `PartialEquationError` skip from a structural skip. `None` here was never exclusively the partial-equation case (an unreconstructible target and a composite-less module link both produce a benign `None`), so a richer return -- not "treat `None` as unscoreable" -- is required; surfacing the signal through the return value keeps it consistent under salsa caching, since the accumulator already replays the one `Warning` and the caller re-reads the memoized verdict on every evaluation. An `Unscoreable` verdict records the `(from, to)` edge so dependent loop scores are DROPPED, at edge (not per-shape-name) granularity: a loop hop resolves to one shape's name, and dropping the loop is conservative but never silently wrong. The two sibling shaped/per-element call sites (`try_disjoint_dim_arrayed`, `try_scalar_to_arrayed`) get the same treatment so the contract is consistent across every site that can doom. The `PartialEquationError` terminal is unreachable through any compiling model today -- every shaped-path doom is recovered by `shaped_guard_form_text`'s changed-last fallback or pinned to a concrete element upstream, and a full-suite probe confirmed no fixture reaches it -- so the contract is exercised with a test-only `ForcePartialEquationError` edge override and tiny fixtures (the `AggLoopBudgetGuard` discipline), covering all three call sites plus discovery mode.
The prior commit (5505dac) claimed its unscoreable-edge recording was "consistent across every site that can doom" and that the PartialEquationError terminal was "unreachable through any compiling model"; both claims were wrong. The agg-half emitters (emit_source_to_agg_link_scores, emit_agg_to_target_link_scores, the shared iterated_feeder_row_scores) and emit_per_element_link_scores all warn-and-skip without recording, and the feeder leg is LIVE: the square-source duplicate-dim fixture (square_source_duplicate_dim_feeder_scores_are_loudly_skipped) dooms its frac -> $ltm-agg feeder rows through the GH #743 UnfreezablePartial machinery and emitted 5 loop scores referencing the never-emitted per-row names -- warned constant-0 stubs, the exact #758-contract cascade GH #780 exists to prevent. That fixture now also pins the flip: dropped loops, no fragment-failure cascade, the loud feeder warnings intact. The unreachability claim survives only for the shaped-query and per-element/scalar-to-arrayed terminals (comments rescoped accordingly). The set threads into both agg-half emitters and the per-element emitter; an edge is keyed exactly as a loop's links spell the hop -- (from, agg.name) for the source half, (agg.name, to) for the target half -- which traverses_unscoreable's subscript-strip matches. A per-ROW doom records at EDGE level and discards the edge's already-built vars (collect-local in the agg->target arms, Some(vec![]) from the feeder emitter, a vars_start truncate in emit_per_shape_link_scores -- also closing the review's orphan-sibling-shape asymmetry): the drop machinery matches stripped (from, to) pairs, so per-row granularity is not implementable, and a partial emission would leave the doomed rows' loops as stubs. Conservative, never silently wrong. The discovery-mode test's rationale was also wrong -- the pinned-loop pass consumes unscoreable_edges in discovery mode too (mod.rs traverses_unscoreable); a new discovery+pinned test pins that consumer, and the scalar->arrayed test now asserts exactly one warning.
The GH #780 doom sites warned unconditionally and then inserted into unscoreable_edges unconditionally, so a re-visit of a doomed edge emitted the identical warning twice. The re-visit is reachable: the pinned pass dedups only edges that EMITTED a var (emitted_edges), and a doomed edge emits none -- so discovery mode plus a pin through the doomed edge runs the emitter twice and it re-dooms deterministically (two identical driver -> grid warnings on the scalar->arrayed fixture, pinned by the new gh780_doomed_edge_pinned_revisit_warns_once test). The pre-existing #758 sites already gate the warning on the insert returning true; the new sites now follow the same convention. Nine warn-then-insert sites become warn-only-on-first-insert (the per-element emitter's two dooms, the feeder-row and scalar-feeder dooms, and the five agg->target dooms). try_scalar_to_arrayed instead early-returns the loud-decline shape when the edge is already recorded: its warning fires inside the build_var closure BEFORE the caller's recording, so insert-gating would need to restructure the closure, while the early return also skips the wasted per-element re-generation. The shaped-query sites need no change -- their warning rides the salsa accumulator, which collects once per query instance regardless of call count.
The BARE-spelled per-row coefficient of an un-hoisted multi-source reducer (`growth[D1] = SUM(matrix[D1,*] * frac)` with `frac` arrayed and referenced without a subscript) got a silently-wrong, unwarned, sustained loop score (~3x for SUM). Execution iterates the bare `frac` over the target's D1 dimension (slot r reads frac[r]), but the read-slice vocabulary cannot express a bare iterated read, so the reducer stays un-hoisted and the GH #743 changed-last chooser falls to a per-element partial `sum(matrix[d1,*] * PREVIOUS(frac))` that the engine pins per slot -- disagreeing with the broadcast execution. The frac-closure loop then carried a sustained ~2.92 with no warning, violating epic #488's no-silent-wrong- numbers invariant. The changed-last leg of `shaped_guard_form_text` now detects a bare arrayed source referenced inside an array-reducer argument and returns `UnfreezablePartial`, riding the GH #780 `Unscoreable` plumbing: the shaped query warns once and records the edge, so dependent loop scores are DROPPED rather than stubbed. The decline keys on "bare arrayed ref inside a reducer", not bare arrayed refs generally, so the adjacent shapes are untouched: the subscripted spelling `frac[D1]` stays hoisted (GH #767/T5), a bare arrayed ref outside any reducer keeps its compiling changed-first partial, scalar feeders keep their `generate_scalar_feeder_to_agg_equation` path, the #526 transposed-dep changed-last is unaffected, and RANK (array-valued, never hoisted) is excluded. The decline covers the whole scalar-collapsing reducer class (SUM/MEAN/MIN/MAX/STDDEV) and holds in discovery mode. C-LEARN's LTM var count is unchanged (it contains no such shape).
Adversarial review of ef91194 found the implementation sound but the fresh prose factually wrong in ways that would misdirect the future option-1 (slice-vocabulary) implementer. The walker rustdoc and decline comment claimed execution cleanly iterates a bare arrayed reducer feeder over the target's dimension (slot r reads source[r]); an asymmetric probe shows execution actually computes growth[r] = |D1| x sum_d2 matrix[r,d2] x frac[r] -- a spurious cardinality factor, tracked as engine issue GH #789. The prose now states the bare spelling's execution semantics are themselves anomalous and that the changed-last partial provably disagrees with whatever execution computes, without asserting an iteration the engine does not implement. The same correction lands in the integration test docs, and the scalar-feeder unit test's self-contradicting parenthetical is rewritten to state the actual contract (scalar source => gate inert via empty source_dim_names => changed-last governs; the whole-RHS scalar-feeder spelling's separately-tracked defect is GH #790). The decline also gets a shape-specific diagnostic: a new PartialEquationErrorKind::BareReducerFeeder names the bare arrayed reference inside a reducer argument and the subscripted-spelling workaround (e.g. frac[D1]) instead of reusing the generic unfreezable text, since the generic wording describes a freeze hazard this shape does not have. The compile.rs unreachability comment for the shaped query's PartialEquationError terminal is rescoped -- the gh779 decline made it live-reachable, and the test-only force seam is retained for surgically dooming arbitrary edges. The walker rustdoc documents that the reducer set (reducer_collapses_to_scalar) includes SIZE harmlessly, and a new pin (whole_rhs_bare_reducer_stays_scored) locks the whole-RHS bare reducer's try_cross_dimensional scoring against future over-decline.
Pin the current element-graph edge output for two agg-routed shapes ahead of the GH #783 I4 single-derivation refactor of emit_agg_routed_edges' source-row enumeration: - A square source whose iterated axes repeat a target dim (out[D1] = base[D1] + SUM(cube[D1,D1,*])): the source->agg half routes each row to its literal [D1,D1] slot (the full cartesian, off-diagonal included -- causally correct). The agg->out expand_same_element half is pinned at its current off-diagonal-emitting shape, which is the GH #778 phantom; the test comments that assertion so the eventual #778 fix flips a recorded expectation rather than landing in silence. - An iterated-dim projection feeder (frac[D1] in SUM(matrix[D1,*]*frac[D1])) routed by its OWN all-Iterated slice (1:1 row->slot), distinct from the canonical reduced source's slice -- exercising the per-source-slice path of the row derivation about to be unified. These tests pass byte-identically on the pre-refactor code; the refactor must keep them green.
emit_agg_routed_edges enumerated its agg-routed element-edge rows via a private AxisPlan struct and its own cartesian product over per-axis element lists, parallel to read_slice_rows (which the link-score emitters consume). The two enumerations agreed today but were independent -- proven by a mutation experiment (GH #783): widening Reduced.subset to the full extent inside read_slice_rows left the production element edges unchanged while only the link-score side moved. That is the two-surface-divergence class the shape-expressiveness design's invariant I4 (single derivation) exists to forbid: the rows a source contributes per result slot must be computed ONLY by one derivation, not re-derived from from_dims cartesian products by each consumer. Extract read_slice_row_parts in db/ltm/loops.rs: the structured low-level derivation returning (row_parts, slot_parts) per source row. read_slice_rows becomes its comma-joined + co-reduced-grouped projection (consumed by the link-score emitters), and emit_agg_routed_edges' source->agg arm now reads the parts directly -- so it can format_element_name the row and subscript the agg node from the slot parts without a re-join/re-split round trip. Parts rather than joined strings is load-bearing: a canonical element name can contain a comma (a quoted dimension element like "a,b"), so joining then splitting on ',' would be ambiguous; sharing the Vec<String> parts is the only representation that provably cannot drift. The decline condition is byte-equivalent to the old planned/read_slice_ok gate (same arity check -- from_dim_element_lists.len() == from_dims.len() -- and the same iterated_axis_slot_elements None), so the conservative "every source element -> scalar agg" fallback engages identically, and the link-score emitters share that exact None so their fallback fires in lockstep. The scalar-feeder arm, the agg->to expand_same_element half (where the GH #778 square-source phantom lives, untouched here), and the debug_asserts are preserved. Mutation re-verified post-refactor: widening Reduced.subset inside read_slice_row_parts now fails BOTH the element-graph subset tests AND moves the link scores (the out-of-subset row's link_score:arr[c]->agg appears on both surfaces together), confirming the drift channel is closed. Reverted the mutation; no behavior change ships. The four proptest regression seeds committed in proptest-regressions/db/element_graph_proptest.txt were captured during this mutation experiment (they fail instantly under the I4-divergence mutation), so they guard the shared-derivation class rather than historical production bugs; the seed file carries a provenance comment saying so. The element_graph_proptest subset/mixed oracles, which used to be 'accidentally differential' against the production AxisPlan enumeration, are now shared-derivation consistency checks (both walk read_slice_row_parts); their comments are corrected and the deterministic literal-pin companions (plus, for the subset family, the ltm_array_agg integration tests' numeric pins) are noted as the independent oracles. The proptest was deliberately NOT changed to consume any new helper.
A reducer whose Iterated axes repeat a target dim -- `out[D1] = SUM(sq[D1,D1,*])`, the inline `base[D1] + SUM(sq[D1,D1,*])`, or with a feeder `1 + SUM(cube[D1,D1,*] * frac[D1,D1])` -- mints (pre-fix) a synthetic agg arrayed over result_dims == [D1,D1]. The executed A2A simulation reads only the DIAGONAL (sq[e,e,*] per target slot e), but the LTM machinery enumerated the full square, and every per-axis emission path pins subscript indices BY DIM NAME, ambiguous across the two D1 occurrences. The three halves disagreed: the element graph fanned out the off-diagonal agg->target edges (gh778), the link-score projection kept only the diagonal, and the co-source row partials emitted CONFIDENT per-(row, slot) scores on the off-diagonal rows the simulation never reads -- reaching the results slab UNWARNED (gh785's live silent-wrong-number on the link surface). PR #787 defended only the feeder half. Decision: DECLINE the whole shape at the single agg-minting gate rather than teach every per-axis path to resolve a repeated dim positionally (the larger "diagonalize the axis into the AxisRead vocabulary" alternative, unjustified for this rare shape). result_dims_has_repeated_dim gates both enumerate_agg_nodes mint sites (synthetic via walk_subexpr_for_aggs, variable-backed via walk_var_equation), keyed on result_dims -- the single place the canonical slice's Iterated target dims are derived -- so all halves and both surfaces (edges and scores) inherit one decision per the epic's "two-surface decisions share one predicate" invariant. Evidence the decline alone is insufficient (probed numerically with executed sims, both modes): declining the hoist routes the co-source into try_cross_dimensional_link_scores' cartesian partial-reduce branch, whose own from_pos name->position map collapses the duplicated dim to ONE position (last-match), emitting confident scores like cube[r1,r2,*]->x[r2] for the off-diagonal rows the simulation never reads -- STRICTLY WORSE (24 phantom loops vs 12, still WARN count=0). So the cartesian-branch landing is closed in lockstep: emit_unscoreable_duplicated_dim_source_warning loud- skips a from-dim-repeated source whose duplicated dim survives as a result axis (the gh758/gh780 discipline -- no link-score var, edge recorded unscoreable so loops drop). The element graph keeps its conservative full cross-product (a sound superset), so edges and scores stay coherent. Placement is load-bearing: the loud skip sits AFTER the agg branch, so the LIVE gh767 repeated-SOURCE-dim shape (growth[D1] = SUM(matrix[*,D1] * frac[D1]) over matrix[D1,D1], slice [Reduced, Iterated], result_dims=[D1] -- NOT duplicated) stays hoisted and correctly scored. Post-fix all three spellings, in BOTH exhaustive and discovery modes, emit no phantom co-source score, one warn-once edge-level Warning per duplicated-dim source (cube, plus frac in the feeder spelling), zero loop scores through the shape, and finite emitted series. The scalar-target full reduce of a square source is NotSimulatable (engine rejects it), so the partial-reduce scope is complete. Defense-in-depth: pin_iterated_dim_indices' #787 ambiguity bail and resolve_mismatched_index_position's repeated-dim arm are now unreachable for this shape (no square-source agg is minted), but retained as structural guards; their rustdocs and the gh780/compile.rs reachability notes are updated to cite this change. Test flips: - element_graph_square_source_duplicated_dim_routes_full_cartesian -> element_graph_square_source_duplicated_dim_declines_to_cross_product (element_graph_tests.rs): the golden-pinned off-diagonal agg->out phantom edges are inverted; the test now asserts NO agg node and the conservative cube[*]->out[r1],out[r2] cross-product. - square_source_duplicate_dim_feeder_scores_are_loudly_skipped -> square_source_duplicate_dim_reducer_is_loudly_skipped (ltm_unified_tests.rs): the whole topology changes (no agg minted); rewritten to assert BOTH cube and frac edges loud-skip, no per-element link score for either, zero loops, no fragment-failure cascade. - gh780 reachability comment (ltm_unified_tests.rs) repointed from the feeder-half terminal to the cartesian-branch terminal. New tests (RED state confirmed empirically via probe before the fix): - square_source_{whole_rhs,inline,with_feeder}_declines_and_skips_loudly (ltm_array_agg.rs): the three spellings x both modes, asserting the post-fix contract. Non-square shapes are untouched: byte-identity is the existing suite (4484 lib + 248 integration green), and the gh767 repeated-source-dim fixture (repeated_dim_co_source_pins_feeder_at_iterated_axis) still compiles and scores cleanly.
…ening Review follow-up to d35bbd2 (the GH #778/#785 square-source decline), six doc/diagnostic/test items, no logic change: - Scope the two "unreachable defense-in-depth" comments on live_read_slice (try_cross_dimensional_link_scores and emit_source_to_agg_link_scores) correctly: the repeated-dim CO-SOURCE shape with slice [Reduced, Iterated] (matrix[D1,D1] read as SUM(matrix[*,D1] * frac[D1]), result_dims [D1]) is the LIVE GH #767 shape -- still minted, and the positional resolution remains load-bearing for it (pinned by repeated_dim_co_source_pins_feeder_at_iterated_axis). Only the doubly-Iterated case (result_dims repeated) is declined, matching resolve_mismatched_index_position's rustdoc. - Correct variable_backed_shape_is_expressible's rustdoc: an owner declared over the same dim twice (x[D1,D1] = SUM(cube[D1,D1,*])) is NOT a malformed declaration -- it compiles and simulates (each slot reads its own full row), reaches walk_var_equation's mint gate with ALIGNED iterated dims, and it is the result_dims_has_repeated_dim check itself, live and load-bearing, that declines it. New integration test square_owner_whole_rhs_declines_and_skips_loudly pins that spelling (loud skip on cube -> x, loops dropped, exactly one warning, both modes). - Reword emit_unscoreable_duplicated_dim_source_warning's user-facing message and rustdoc to describe the actual predicate (a source whose declared dimensions repeat a dimension that survives as a result axis cannot be scored per-element -- the result coordinate is ambiguous between the occurrences) instead of asserting diagonal-only reads, which is wrong for two spellings the skip correctly catches: the repeated-dim owner (full-square reads) and the whole-extent broadcast (x[D1] = SUM(sq[*,*,*]), no iterated axes). For the record, d35bbd2's "non-square shapes are untouched" claim missed that the whole-extent broadcast over a square source ALSO flipped there -- from confident cartesian per-(row, result) scores (whose single-slot attribution was wrong: every row feeds every slot) to the loud skip, strictly safer; re-verified empirically in this follow-up. - Fix the misnamed warning fn in result_dims_has_repeated_dim's rustdoc (emit_duplicated_dim_source_unscoreable_warning -> emit_unscoreable_duplicated_dim_source_warning). - Strengthen element_graph_square_source_duplicated_dim_declines_to_cross_ product's "no agg node" assertion from exact-name membership to a substring check over both edge sides, so subscripted agg[r1,r2] slot nodes would be caught directly. - Fix the stale agg-branch fallback comment (a stale-invariant None from read_slice_rows for a repeated-from-dim source now lands on the loud skip, not the cartesian derivation), and restore reducer_source_vars gating in walk_subexpr_for_aggs so combined_read_slice runs only for actual hoistable reducer Apps (d35bbd2 had it computed for every non-in_reducer App -- behavior-neutral wasted work).
A whole-RHS variable-backed reducer whose slice has no Iterated axis (Pinned and/or subset-Reduced only) but whose owning variable is arrayed -- share[Region] = SUM(pop[nyc,*]) -- collapses to one scalar value that broadcasts over every owner element. It was the deferred residual of the shape-expressiveness phase: declined by the variable_backed_reduce_agg gate and loud-skipped by try_cross_dimensional_link_scores, so every loop through the edge was dropped. This completes the design's section-6 broadcast rule (the section-3 PerElement rule applied to a reducer owner). The fix widens the single gate that all three LTM surfaces consume, so edges, scores, and loop routing move together by construction: - variable_backed_reduce_agg admits the arrayed-owner scalar-result slice (the else arm returns true instead of to_dims.is_empty()). The minting path is untouched -- the agg already exists (no Iterated axis means the variable IS the agg), and the d35bbd2 square-source decline still fires upstream at result_dims_has_repeated_dim, so it is not disturbed. - emit_agg_routed_edges fans each read row across the owner's FULL target element set (pop[nyc,d2] -> share[e] for every e) instead of landing on the bare, nonexistent agg node -- in both the primary and the defensive fallback arm. - emit_broadcast_reduce_link_scores (new) emits the matching per-(read-row, full-target-element) scalar scores via the existing generate_element_to_reduced_equation builder (target pinned to e, the co-reduced slice the full read-row set). It fires before the result_axis_names containment check, so the related-dim (share[Region]) and disjoint-dim (share[D9]) spellings -- the read rows are independent of the owner's dims -- are scored identically. No spelling is silent-wrong: the disjoint hop's loop routing needed a new is_broadcast_reduce_edge predicate in the mixed-branch link builder (where is_partial_reduce_edge's dim-containment check fails), without which the loop would reference the nonexistent Bare from->to[e] name and stub to zero. The score derivation reuses the same read_slice_rows that the element graph uses, so the per-(row, e) names are exactly the hops the per-circuit loops reference; loop routing through representative_has_partial_reduce_hop already keys on the widened gate. The emit_unscoreable_broadcast_reduce_- edge_warning loud-skip is now dead and deleted. Tests: the loud-skip pins flip to positive scoring -- Pinned and subset shapes with exact 0.5 link scores and 0.5 loop scores in a feedback loop; a disjoint-dim variant closed through a scalar reducer; the element-edge pin asserts exactly (read rows x all target elements) with no unread-row edges; a forced-discovery twin verifies the per-(row, e) reducer names parse symmetrically and trace the real same- and cross-element broadcast circuits with no phantoms.
Comments-only follow-up to 0fcfba5 closing the review's four doc-staleness items; no logic changes. - analysis.rs Direct-dispatch comment: the gate now ADMITS the arrayed-owner scalar-result broadcast slice (this dispatch is where its element edges come from), so the "None for the GH #777 loud-skip arm" sentence is rewritten to enumerate the broadcast admission, mirroring the gate rustdoc. - link_scores.rs emit_link_scores_for_edge: the Some(vec![]) contract no longer includes the deleted broadcast loud decline; it now enumerates the surviving cases (SIZE/Constant skip, the GH #780 feeder doom, the GH #778/#785 duplicated-dim skip). The function-level return-contract sentence on try_cross_dimensional_link_scores gets the same fix, and emit_broadcast_reduce_link_scores' rustdoc is corrected to match the code: a read_slice_rows decline returns None via the ? (propagating to the dispatcher's later landings), not Some(vec![]) -- unreachable defense, since a Pinned/Reduced-only slice has no Iterated remap to fail and arity is invariant I2. - loops.rs representative_has_partial_reduce_hop: the "only the Iterated-armed acceptance is reachable here" claim is false post-gh777; both arrayed-owner acceptances (aligned partial reduce and the no-Iterated broadcast) route all-subscripted circuits here. - loops.rs is_broadcast_reduce_edge: document that the predicate never inspects from's own slice, so it also matches a scalar-feeder edge into a broadcast agg -- inert at the sole call site, which is guarded by from_subscripted && to_subscripted.
An I1-declined multi-source whole-RHS reducer with mismatched co-source slices -- `share[Region] = SUM(pop[nyc,*] * w[*])`, where `pop`'s slice `[Pinned(nyc), Reduced]` disagrees with `w`'s `[Reduced]` so `accept_source_slices` mints NO variable-backed agg -- fell through `try_cross_dimensional_link_scores` to the legacy cartesian partial-/full-reduce derivation. That projection ranges over EVERY `from` element by declared dimension positions, so for a strict slice it emitted confident silent link scores INCLUDING for rows the reducer never reads (`pop[boston,p]->share[boston] = 1.0`) and mis-divided the read rows (`pop[nyc,p]->share[nyc] = 1.0` vs the true SUM contribution share 0.5, the un-pinnable mismatched-arity body dooming the changed-first partial to the |dz/dz| = 1 fallback). Only the loop surface degraded loudly; the link surface carried unwarned wrong numbers, violating epic #488's no-silent- wrong-numbers invariant. The scalar-target arm (`total = SUM(pop[nyc,*] * w[*])`) was even worse -- silent on every surface. The fix declines the strict-slice family with the #758/#780 loud skip: one warning naming the edge, no link-score variable, the edge recorded in `unscoreable_edges` so loops through it drop. The decline re-derives `from`'s read slice via the SAME per-axis classifier the hoisting path uses (`unhoisted_reducer_source_read` over `compute_read_slice`/ `classify_axis_access`), so the decline predicate and the agg-minting predicate are one source of truth and can never disagree about a full-extent read. Predicate: decline ONLY when `from` is read strictly (a Pinned element or a subset-Reduced axis) AND never at full extent. The "never at full extent" clause is load-bearing for the blast-radius boundary: - I1-declined mismatched co-source (`SUM(pop[nyc,*] * w[*])`): pop read only as `[Pinned, Reduced]` -> decline (the fix). - GH #744 self-reference (`SUM(pop[*] * pop[north])`): pop ALSO read at full extent (`pop[*]`), so no row is unread -- the per-row multi-slice ambiguity is the deliberately conservative delta-ratio fallback, NOT the unread-rows defect. Kept on the cartesian path (pinned by fixed_literal_self_reference_rows_fall_back_consistently). - #779 bare-feeder matrix edge (`SUM(matrix[D1,*] * frac)`): matrix read at `[Iterated, Reduced]`, a full-extent read -> its correct diagonal scores stay (the separate bare-feeder `frac` decline is unchanged). - Dynamic-index reducer (`SUM(pop[idx,*])`): not statically describable -> NotDescribable, keeps the documented conservative cross-product (pinned by element_graph_dynamic_index_reducer_stays_conservative, test_disjoint_dynamic_index_loop_scores_dropped). - Element-mapped declined reducers (#756): the result-axis containment check in try_cross_dimensional_link_scores already returns None before the cartesian arm, so they never reach the new gate (declined_element_mapped_reducer_edge_skips_loudly). try_scalar_to_arrayed_link_scores has no analogous fallthrough: it requires a scalar `from`, and the strict-slice family always has an arrayed reducer source. Discovery mode and the pinned-loop pass share this emitter, so the decline holds in all three (pinned by the new discovery-mode test). C-LEARN structural and var-count guardrails unchanged (it contains no strict-slice multi-source reducer).
Four items from the c1a16df review, in one commit: 1. The decline warning interpolated "e.g. {from}[nyc,*]" with "nyc" HARDCODED from the test fixture -- nonsense for a user whose dims are {china, usa}. UnhoistedSourceRead::StrictSlice now carries the representative strict slice (the first strict read in deterministic walk order), and the message renders it via render_read_slice_for_diagnostic (Pinned -> element, Iterated -> source dim, Reduced -> "*", subset -> "*:{elems}" -- the AxisRead vocabulary carries a subset's elements, not the subdimension's name). The integration test pins the rendered "pop[nyc,*]" so a regression to a canned example fails. 2. unhoisted_reducer_source_read called the UN-tracked reconstruct_model_variables (O(all model vars)) once per edge reaching the cartesian fallthrough -- its first per-edge caller. It is now a salsa-tracked query keyed on the interned LtmLinkId (the link_score_equation_text idiom), bounding the reconstruction to once per (edge, revision) so pinned-pass and discovery re-visits are cache hits. Tracking was chosen over threading the caller's variable map because try_cross_dimensional_link_scores holds only SourceVariable handles (not reconstructed Variables with ASTs); threading would have forced a second dims-lookup vocabulary into the AggWalkCtx walkers. The choice is documented on the query's rustdoc. 3. The warning's "the only remaining derivation would score {from}'s unread rows" overstated for the mixed FixedIndex-site + strict-reducer-read shape (a per-site derivation exists in principle; the edge-granularity drop is deliberate conservatism). Softened to "the whole-edge cartesian derivation that remains here would ... so the edge is declined instead", with the scoping documented on the emitter's rustdoc. Scope note for the record: c1a16df's "closes the strict-slice family" claim covers the NO-AGG leg only -- the agg-routed leg is tracked as gh793 and the per-element-owner leg as gh792, both filed by the reviewer and queued. 4. The walker rustdoc claimed it "mirrors walk_subexpr_for_aggs' maximal-reducer rule", which was imprecise: the real walk descends a DECLINED outer reducer with in_reducer unchanged (nested reducers stay hoistable) while this one suppresses nested collection under any hoistable-kind outer reducer. The doc now states the divergence and why it is unobservable at the gh791 gate (every divergent case routes the nested hoisted reducer's reads through its own agg, never the cartesian fallthrough for this edge).
The scalar feeder of a whole-RHS variable-backed reduce (`growth[D1] = SUM(matrix[D1,*] * scale)`) reached `try_scalar_to_arrayed_link_scores`, whose per-target-element generator emitted `scale->growth[a]`/`[b]` partials referencing the lagged wildcard slice (`SUM(PREVIOUS(matrix[d1.a,*]) * scale)`, a GH #541-class fragment the compiler rejects). Every such fragment kept a layout slot but no bytecode, so the feeder scores read a constant 0 and every loop through the feeder degraded to a warned 0-stub -- the wrong degradation mode, since the SUBEXPRESSION spelling (`0.1 + SUM(matrix[D1,*] * scale)`, synthetic-agg-backed) scores the same dataflow correctly via `emit_source_to_agg_link_scores`' scalar-feeder arm. The routing decision: the edge never routed `ThroughAgg` because a variable-backed agg mints no synthetic node, so the IR classified the scalar reference `Direct` and the dispatcher fell through to the scalar->arrayed per-element path. Rather than add a loud skip, route the feeder FIRST -- to the SAME changed-last convention the synthetic-agg arm uses: ONE Bare A2A score `$:ltm:link_score:scale->growth` over the agg's `result_dims` via `generate_scalar_feeder_to_agg_equation`. The loop builder needs no change -- `loop_link_score_ref` already prefers the A2A name and subscribes it after the quote (`"...scale->growth"[a]`), so the whole-RHS and subexpression spellings now produce byte-equivalent feeder scores (+1) and loop scores (~0.5). The new `ltm_agg::scalar_feeder_of_variable_backed_agg` predicate is the scalar sibling of `AggNode::source_is_projection_feeder` (GH #767's arrayed iterated feeder): a scalar feeder's value is constant across the co-reduced slice, so its single A2A score suffices. Gated on the shared `variable_backed_reduce_agg` decision the element graph and loop builder consult, so the emitted name is exactly the hop the per-slot loops reference. MEAN/MIN/MAX, multiple scalar feeders, and a scalar feeder alongside an iterated projection feeder all ride the same reducer-agnostic path.
Three items from the adversarial review of 0cae644. The major one: the empty-result_dims arm of the new scalar-feeder branch claimed unreachability for an arrayed target, but the GH #777 broadcast slice reaches it (`share[D9] = SUM(matrix[a,*] * scale)`: Pinned slice, no Iterated axis, result_dims empty, owner arrayed -- the broadcast arm of variable_backed_reduce_agg admits it). The Equation::Scalar emission referenced the bare multi-slot owner, failed assembly (3 warnings, constant-0 series), and left the edge out of unscoreable_edges -- the exact warned-0-stub middle state gh790 was filed to eliminate, one shape over. Fixed by correct emission rather than a loud doom: the score is ALWAYS ApplyToAll here (to_dims is non-empty in this function), over result_dims for the aligned shape and over the OWNER's dims (datamodel-cased via the same dm_dims mapping link_score_dimensions applies) for the broadcast one. The changed-last text compiles in both: the bare owner element-resolves inside its own A2A context and the frozen Pinned-slice reducer body is scalar-valued. Verified +1 per slot / ~0.5 loops, pointwise equal to the subexpression spelling (`0 + SUM(...)`) of the same dataflow. compile_directly now true with a corrected comment: what actually compiles the text verbatim is the non-empty-dimensions A2A arm of compile_ltm_synthetic_fragment (the old comment wrongly credited the synthetic-agg check -- growth is not a synthetic agg name); the direct flag defends against the (from, to)-keyed salsa path re-deriving a different changed-first equation under any future dims-handling change. Test honesty and strength: the discovery twin is renamed to emits_cleanly_in_discovery_mode with a scope note -- it asserts emission and warnings only, and the scalar-from Bare A2A name is currently structurally undiscoverable in the search graph (the pre-existing expand_a2a_link_offsets gap; the discovery offsets fix is queued separately). The spelling-agreement test gains a TIME-VARYING matrix case (matrix = pop[D1] * 0.05, `0 +` subexpression prefix so the dynamics are byte-identical): the feeder score genuinely departs from +/-1 there (vacuity-guarded), so the agreement pins the changed-last convention itself, not just the constant-matrix analytic constants; all 6 loops (2 feeder + 4 co-source circuits) agree pointwise across spellings.
A Bare A2A link score is dimensioned over the TARGET's dims, but discovery's expand_a2a_link_offsets subscripted BOTH endpoints with the score's full element tuple. When the source's own declared dims were fewer than the score's, that minted a from-node naming no real element-graph node -- a scalar feeder (scale->growth, the gh790 routing) invented scale[a]/scale[b] where the real node is bare scale; a lower-dim feeder (boost[Region]-> growth[Region,Age]) invented boost[r,a]; a positionally-mapped pair (x[Region]->target[State]) invented x[s]. The edge dangled in the search graph and every loop through such a feeder was silently undiscoverable in discovery mode. Post-gh790 the scalar leg was the must-fix: the broken per-element scores compile cleanly there, so discovery showed zero warnings while the feeder loop was simply absent. The fix subscripts only the TARGET node per element and PROJECTS the SOURCE node onto from's OWN declared dims, reusing the element graph's own db::expand_same_element rule: bare for a scalar feeder, the same-element diagonal for an equal-dim feeder, the broadcast/partial-collapse form for a lower-dim feeder, and the positionally-mapped diagonal for a mapped pair. The target-element offset stays keyed on the target's row-major slot (score dims == target dims for every Bare A2A retarget), so each projected from-node attaches to the right result column. Consuming the same expand_same_element the element graph emits keeps the two surfaces' node spelling identical by construction rather than via a parallel rule that could drift. The from-var declared dims plus the dimension-mapping context ride in a new LinkExpansionContext that analyze_model builds via the public build_link_expansion_context (the same variable_dimensions / project_dimensions_context queries the element graph reads); the db-less discover_loops convenience path passes the default empty context (no A2A expansion runs there). Element-mapped (non-positional) pairs are out of scope and need no handling: link_score_dimensions already declines to retarget them to the target's dims under the gh756 positional-only gate, so no Bare A2A score is emitted for them (they take the gh758 loud skip) and no phantom can reach the projection. Only positional mappings -- gh527's live case -- flow through, in lockstep with mapped_element_correspondence. A pre-existing discovery completeness gap surfaces as a bonus: the gh525 twin's row_sum partial-collapse + broadcast circuits, previously dangling at the row_sum hop, are now discovered (8 loops, not 4); its stale "KNOWN residual" note is removed.
Review hardening for the gh754 projection fix. The gh790-family integration fixtures are slot-symmetric (constant co-sources score the feeder identically +1 in every slot), so a mutation rotating each expanded edge's result slot by one passed all four committed integration tests; only a parse_link_offsets unit test caught it. Add the defense-in-depth layer the review validated: an asymmetric fixture (growth[D1,D2] = SUM(m3[D1,D2,*] * scale * pop[D1,D2]) with per-element-distinct m3, whole-RHS reduces only so no synthetic agg nodes separate the reported links from the scored links) whose Bare A2A scores are per-slot distinct on BOTH projection arms, plus a per-loop oracle asserting each discovered loop's score series equals the product of independently slot-resolved link-score series pointwise (slots derived from the datamodel row-major rule, not via parse_link_offsets, so the oracle cannot agree with a misattribution by construction). A fixture-asymmetry guard pins the per-slot distinctness so the fixture cannot silently degenerate back to slot-symmetric. Mutation-validated: rotating either arm's offsets by one fails the test at the first divergent step; production code unchanged. Also rewords the now-historical "Bare-A2A form was undiscoverable" note in simulate_ltm.rs (post-gh754 a scalar-source Bare A2A score IS discoverable; the per-target-element emission for plain scalar->arrayed edges remains the deliberate convention on its own merits) and notes expand_same_element's second consumer (discovery's from-node projection) in its rustdoc so a future semantics change there knows discovery consumes it.
…gh792) The per-element-equation (`Ast::Arrayed`) sibling of the gh791 strict-slice shape. An owner whose slots each hold an I1-declined strict-slice multi-source reducer -- `share[nyc] = SUM(pop[nyc,*] * w[*])`, `share[boston] = SUM(pop[boston,*] * w[*])` -- never reaches `try_cross_dimensional_link_scores`' cartesian arm (a per-element owner has no single dt-expression), so it fell through `emit_link_scores_for_edge`'s final fallthrough to `emit_per_shape_link_scores` shape Bare and minted ONE arrayed `link_score:pop->share` simulating to ~-0.0 at every step with no per-edge warning. Only one loop-score fragment-compile failure surfaced; the other enumerated loops compiled and consumed the silent near-zero, an unwarned wrong loop score. The mdl importer expands a single apply-to-all Vensim equation into N per-element equations (gh651), so this spelling is the import-reachable one. The fix extends the gh791 verdict instead of adding a second predicate, keeping the epic's one-verdict-source invariant. `unhoisted_reducer_source_read`'s `Ast::Arrayed` arm -- previously a conservative `NotDescribable` stub -- now classifies each SLOT body with the SAME single-expr classifier the whole-RHS owner uses (factored out as `classify_expr_source_read`) and combines per slot. The verdict is consulted at `emit_link_scores_for_edge`'s final fallthrough, exactly parallel to how the cartesian arm consults it, and reuses the salsa-cached `LtmLinkId`-keyed query (a cache hit when the cartesian arm or a pinned-/discovery-pass re-visit already derived it). The fallthrough is reached only with `routed_aggs` empty, so a `StrictSlice` here is a genuinely un-hoisted strict read whose Bare stand-in is genuinely wrong; it takes the gh758/gh780 loud skip (one warning naming the edge and its actual slice, no link-score variable, the edge recorded in `unscoreable_edges`, dependent loops dropped), holding in exhaustive, discovery, and pinned modes. Any-strict precedence: an `Ast::Arrayed` owner's slots may read DIFFERENT slices, or only SOME slots may read the source. The edge gets ONE arrayed Bare stand-in that conflates all slots, so ANY slot reading `from` strictly proves it wrong for at least that slot -- the combination declines the WHOLE edge with the first strict slot's representative slice. A strict slot dominates a `NotDescribable` slot (the strict slot is provably wrong, so the loud skip is always sound; the dynamic-index slot's cross-product is merely documented-OK). The gh744 full-extent escape stays SLOT-LOCAL: one slot reading `from` both full-extent and pinned classifies `FullExtent` like the scalar path, since one slot's full read says nothing about a different slot's strict read. The working disjoint-dim FixedIndex per-element family (gh510) is byte-identical -- it reads `from[m]` via literal subscripts OUTSIDE any reducer, which the verdict (reducer-reads-only) never collects, so it classifies `NotDescribable` and keeps its scores (pinned by simulate_ltm's AC3.3 disjoint test).
Review follow-up to 676b4e9: two adjacent spellings of the per-element-owner defect were still silently wrong, both root-caused to the same premise error -- the first fix's strict-vs-full-extent distinction validates the CARTESIAN projection, which requires a single dt-expression a per-element (Ast::Arrayed) owner does not have, so no verdict short of "decline" makes the Bare stand-in sound for these owners. Finding 1, the DIM-NAME spelling: share[nyc] = SUM(pop[Region,*] * w[*]) per slot. Execution pins Region inside slot nyc to nyc (verified empirically and now pinned in-test: share[nyc] simulates to 10.0, the strict row-only value, not 20.0), but the first fix passed the owner's declared dims as iterated dims -- enumerate_agg_nodes' own Arrayed arm deliberately uses EMPTY iterated dims -- so the axis classified Iterated -> full extent -> no decline, and the silent ~-0.0 stand-in plus confident 0.0 loops returned byte-for-byte. Finding 2, the FULL-EXTENT multi-source spelling: share[a] = SUM(pop[*,*] * w[*]) per slot (I1-declined, un-hoisted; executed share = 20.0, a genuine whole-pop read) also minted the silent stand-in: FullExtent is safe for scalar/A2A owners only because the cartesian arm scores them, and per-element owners have no cartesian arm. The unified rule replaces per-slot classification for Arrayed owners: if ANY slot (or the EXCEPT default) reads `from` inside a maximal reducer -- describable or not -- the verdict is the new PerElementReducerRead and the edge takes the gh758/gh780 loud skip. Both consultation sites are reached only with the edge's routed-agg set empty, so every such read is genuinely un-hoisted, and the rule subsumes the strict, dim-named, full-extent, AND dynamic-index slot spellings without classification surgery. The dynamic-index sub-case is an explicit decision: pre-fix it was silently stand-in'd exactly like the strict spelling (the scalar/A2A dynamic family keeps its documented conservative cartesian; a per-element owner has none to keep), no existing test pinned the old silent behavior, and declining is consistent -- pinned by a dedicated unit test. Slots referencing `from` only OUTSIDE reducers stay NotDescribable, keeping the disjoint-dim FixedIndex family (gh510, AC3.3) byte-identical. The two consultation sites now share one helper (decline_unhoisted_reducer_edge) so they can never disagree about which verdicts decline; the cartesian site matters for Arrayed owners because an EXCEPT default is the one path classify_reducer admits them there. NOTE the fallthrough gate is not per-element-only: a Scalar/A2A owner whose strict-slice reducer edge falls past the try_* arms (e.g. an equal-dims A2A owner with a reducer subexpression) is declined there too via StrictSlice -- deliberate, the same edge-granularity decline gh791 applies on the cartesian arm, now documented at the gate. Also corrects two false invariant claims from the first fix: the verdict's iterated-dim context now genuinely mirrors enumerate_agg_nodes per AST arm (empty for Arrayed -- the old comment claimed a mirror while diverging, which was exactly finding 1), and the "decline and agg-minting predicates can never disagree" claim is rewritten to its true scope (axis-for-axis agreement for Scalar/A2A owners; a deliberately coarser any-reducer-read rule for per-element owners). The disjoint-dim unscoreable warning no longer misattributes an un-hoisted reducer read as a "dynamic index" (the gh514 reclassification makes the two indistinguishable at that gate, so the message names both).
Route aliased reducer sites by the exact reducer spelling, decline residual unhoisted reducer edges instead of publishing incomplete agg halves, and add a generic loop-score backstop for missing link-score names. The backstop warns and drops the affected loop score before fragment compilation can replace a missing synthetic dependency with zero. Fix the execution-side residuals by deriving Pass1 reducer temp shapes from the transformed expression tree and normalizing temp-array static views to the compact scratch storage that AssignTemp and array-producing builtins actually write. This keeps sliced RANK and active-slot reducer helpers from double-counting dimensions or indexing past temp storage.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #795 +/- ##
=========================================
Coverage 90.87% 90.88%
=========================================
Files 224 224
Lines 140856 142193 +1337
=========================================
+ Hits 128001 129225 +1224
- Misses 12855 12968 +113 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Owner
Author
|
@codex review |
![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: 436e2afbb0
ℹ️ 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
+3146
to
+3147
| if decline_bare_arrayed_reducer_target(db, model, project, from, to, unscoreable_edges) { | ||
| return; |
There was a problem hiding this comment.
Because emit_link_scores_for_edge calls this before any routed/direct score emission for every edge into to, looking up only by target makes one unsafe bare-arrayed reducer suppress unrelated sources too. For example, in an A2A equation like target[D] = local[D] + SUM(pop), the edge local -> target is fully scoreable, but this early return records it as unscoreable just because target also contains the unsafe SUM(pop) reducer. The decline needs to be scoped to reducer sites that actually read from (or delayed until the reducer-shape emission path) so independent direct links still get their link-score variables.
Useful? React with 👍 / 👎.
Keep the phase-one bare-arrayed reducer decline loud for sources coupled to the unsafe reducer, while allowing additive branches whose ceteris-paribus partial never touches that reducer to emit link scores. The predicate deliberately splits only across addition and subtraction: products, divisions, functions, and conditionals still couple the changed source to the unsafe reducer value and must keep the loud decline.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ 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". |
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.
Summary
RANKVM panicTests
cargo test -p simlin-engine --test integration --quietcargo test -p simlin-engine --lib --quietcargo test -p simlin test_get_errors_surfaces_ltm_fragment_failure_after_ltm_sim --test integration --quietgit commitpre-commit hook: dependency policy, docs links, project lint rules, Rust fmt/cbindgen/clippy/tests, TypeScript checks, pysimlin testsFixes #780
Fixes #779
Fixes #783
Fixes #778
Fixes #785
Fixes #777
Fixes #791
Fixes #790
Fixes #754
Fixes #792
Fixes #793
Fixes #789
Fixes #788
Fixes #794