Repository navigation
engine: per-reference LTM element graph and per-shape link scores - #477
Conversation
Captures the principled fix for tech-debt #20 (FixedIndex N-squared edge expansion) and #26 (mixed same-element / cross-element partial equation wrong) in a single AST-walking refactor: emit element edges and partial equations per AST reference site instead of per-(from, to) edge with a single collapsed classification. The conflation between fixed-index references and wildcard reducers under CrossElement is replaced with a RefShape per reference, and the partial-equation builder honors the shape so each link score corresponds to a specific AST occurrence.
Adds explicit AC sections (ltm-per-ref-elem-graph.AC1 through AC6) so the implementation-plan workflow can map each AC case to specific tests and each phase can declare which AC cases it implements and verifies.
Seven-phase plan covering: (1) TDD red tests for the desired contract, (2) AST-walking element graph builder replacing classify_element_dependency + expand_edge_to_elements, (3) per-shape partial equations and per-shape link score variables, (4) loop classification updates with shape annotation on Loop::links, (5) auto-flip threshold measurement, (6) docs and tech-debt closure, (7) final cleanup. Each phase pins acceptance criteria from the design plan.
Resolves all 13 code-review issues from the Finalization step: Critical: - Phase 1: explicit Task 0 production scaffolding (RefShape enum, build_partial_equation_for_shape stub) and Task 0.5 print_eqn canonicalization capture, plus updated module description - Phase 3: full LtmLinkId migration with enumerated call sites; switch to alternative tracked-function approach to minimize churn Important: - Phase 1/2/3/4/5/7: replace --amend / --allow-empty pre-commit gates with direct bash scripts/pre-commit invocations - Phase 3 Task 4: always-suffix Wildcard/DynamicIndex (resolves both has_collision name-stability and AC3.2 reconciliation; design plan AC3.3 updated correspondingly) - Phase 4 Task 3: document heuristic vs. shape-threading decision and add Task 3.5 for edge-aliasing regression coverage - Phase 2 Task 2: Expr2 variant arity corrected to include trailing ArrayBounds/Loc payload fields - Phase 2 Task 3: explicit delegation to expand_same_element Minor: - Phase 5 Task 3: clearer before/after measurement instructions - Phase 6 Task 5: explicit AC8.5 text replacement - Phase 1 Task 3: proptest runs unmarked from day 1 - Phase 4 Task 1: Link.shape struct addition stays in Phase 3, Phase 4 only annotates real values
Resolves all 4 issues from round-2 code review: Critical: - Phase 3 `has_collision` references purged from AC3.3, Tasks 5c/5d/6, and Done When; Phase 4 Task 3 updated to 3-arg link_score_var_name - Naming convention is now consistently the ALWAYS-suffix form across all referenced sites Important: - Phase 1 Task 3 contradiction resolved (proptest unmarked from Day 1) - Phase 3 Task 5d implicitly cleaned up with C-NEW-1 fixes Minor: - Phase 3 Task 8 and Phase 4 Task 7 `amending HEAD` prose replaced with direct `bash scripts/pre-commit` invocations
Phase 1, Task 0 of the LTM per-reference element-graph refactor: add the production scaffolding the new red tests need to compile. Both items are gated with #[allow(dead_code)] until Phase 2/3 populate their use-sites. RefShape classifies the access shape of a single AST reference site (Bare, FixedIndex, Wildcard, or DynamicIndex). build_partial_equation_for_shape is the eventual shape-aware partial-equation builder; today it is an unimplemented!() stub so the Phase 1 tests that exercise it can remain #[ignore]-d while still typechecking against the desired post-refactor signature.
Add a `deps_set` helper inside the existing `ltm_augment` test module so the upcoming Phase 1 tests for `build_partial_equation_for_shape` (Tasks 5 and 6) can construct `HashSet<Ident<Canonical>>` dep sets in one line. The helper canonicalizes each input string via `Ident::new`, matching the wrapping path the per-shape builder will exercise in Phase 3. The helper carries `#[allow(dead_code)]` because its only callers -- the `#[ignore]`-d Phase 3 tests added in the next two commits -- are treated as unused functions by the compiler until Phase 3 removes the ignore. The attribute can be dropped at that point with no other change.
Add two `#[ignore]`d tests that pin the contract for `build_partial_equation_for_shape` over the bare-Var and Wildcard shape families on `share[R] = population / SUM(population[*])`. Both tests fail today with the `unimplemented!()` panic from the Phase 1 stub; they turn green once Phase 3 replaces the body with the real per-shape walker. The expected strings come from Task 0.5's `print_eqn` reconnaissance, documented inline in the phase plan and committed alongside the tests that consume them. The notable canonicalization quirks are: `SUM` -- parsed by the lexer -- round-trips as `sum`, while synthesized `PREVIOUS` keeps uppercase because `wrap_deps_in_previous` builds it as a literal `"PREVIOUS"` `UntypedBuiltinFn`. Phase 3 must follow the same case convention so the per-shape builder's output is byte-equal to what `wrap_deps_in_previous` produces for the wrapped subset. The `deps_set` helper from Task 4 no longer needs `#[allow(dead_code)]` now that these tests reference it; the attribute was dropped at the same time.
Add two `#[ignore]`d tests that pin the contract for `build_partial_equation_for_shape` over the FixedIndex shape on the cross-element migration-pressure equation `(population[NYC] - population[Boston]) * 0.01`. Each test fixes a different element (`nyc`, then `boston`) as the live ref and asserts that the other subscripted ref is wrapped in PREVIOUS(). The element strings inside the `RefShape::FixedIndex(Vec<String>)` variants are lowercase canonical form -- they have to match the AST subscript text after `print_ident` lowercases identifiers via `canonicalize`. Phase 3's `build_partial_equation_for_shape` will need to canonicalize incoming shape element names before comparing them against AST subscript text; the tests pin both that contract and the exact `print_eqn` output (parsed function names lowercase, synthesized `PREVIOUS` uppercase, parens preserved around the subtraction inside the multiplication). Both tests fail today with the `unimplemented!()` panic from the Phase 1 stub.
Two documentation fixes to match Phase 1 docs against the actual function names committed to the codebase. test-requirements.md: AC1.1 and AC1.2 rows listed the test names without their `_truthful` suffix. The implemented functions are `element_graph_fixed_index_broadcast_truthful` and `element_graph_wildcard_reducer_plus_bare_truthful`. AC1.5's name (`element_graph_multidim_partial_fixed_conservative`) was already correct and required no change. phase_01.md Task 5 verification command: the filter `build_partial_equation` matched none of the four partial-equation tests. Replaced with `partial_equation_share`, which matches exactly the two Bare/Wildcard tests that Task 5 introduces. Task 6's `partial_equation_migration` filter was already correct.
Move the RefShape enum from ltm_augment.rs (its Phase 1 stub location) to
db_analysis.rs, ahead of ElementDependencyKind which Phase 2 will retire.
Add a sibling ReferenceSite { source, shape } struct to describe a single
AST reference occurrence -- the new walker (Task 2) will return Vec of
these per (target, source) pair.
ltm_augment::build_partial_equation_for_shape keeps its Phase 3 unimplemented
stub but now imports RefShape from its new home via the db module's
pub(crate) re-export. The dead_code allows on the new types stay until
Task 2's collect_reference_sites makes them reachable from tests.
Add a new walker that descends a target variable's Expr2 AST and emits one ReferenceSite per occurrence of a given source identifier. Mirrors the recursion pattern of the existing classify_in_expr but pushes per-site shapes (Bare, Wildcard, FixedIndex, DynamicIndex) instead of collapsing into a single classification. Subscript shapes are determined by walking the index list: any wildcard yields Wildcard; otherwise every index must resolve via the new resolve_literal_index helper for FixedIndex; anything else falls back defensively to DynamicIndex. The literal helper accepts both element names (parsed as Expr2::Var) and integer literals (Expr2::Const), so both named and indexed dimensions classify cleanly. The walker also recurses into Subscript index expressions so nested references like source_outer[source_inner[*]] still emit a site for the inner reference -- otherwise we would lose causality on the inner var. Four tests in collect_reference_sites_tests cover bare A2A, fixed-index, wildcard reducer, and the mixed bare+wildcard pattern. These run live (not ignored) and pass against the new implementation. Task 4 will consume sites from model_element_causal_edges; until then RefShape and ReferenceSite carry minimal allow(dead_code) stamps so clippy stays clean during this intermediate state.
Add a per-reference edge emitter that translates a (from, to, dims, shape) tuple into the correct element-level edges. This is the per-reference replacement for expand_edge_to_elements: the new walker (Task 2) emits one ReferenceSite per AST occurrence, and Task 4 will hand each site to this helper. Behavior follows the truth table in the design plan. Scalar sources short-circuit to pass-through or broadcast regardless of shape (a scalar source has no subscripts). Arrayed sources branch on the shape: - Bare with arrayed target delegates to expand_same_element so partial- collapse cases (from[D1,D2] -> to[D1]) reuse the existing dimension- matching projection rather than re-implementing it. - FixedIndex pins the source to a single element tuple and broadcasts to the entire target -- only one source key appears in the output. - Wildcard and DynamicIndex emit the conservative full cross product, matching today's CrossElement branch. Three tests cover the scalar pass-through, the FixedIndex single-element narrow-source case, and the same-dim Bare diagonal. The diagonal case is the regression-prone edge of expand_same_element; pinning it here guards against silent breakage during Task 4's pivot.
… emission Replace the per-(source, target) ElementDependencyKind classification with an AST-walking per-reference emission scheme. For each variable-level edge, the new builder collects every reference site of the source in the target's AST, classifies each into a RefShape, and emits the per-shape element edges. Multiple sites of the same shape dedupe naturally because edge values are sets. Per-element correctness for Ast::Arrayed equations required threading a target_element through the walker: when the reference appears inside a per-element subscript expression, the emitter narrows the target node set to the pinned element rather than broadcasting across the full target dimension. The cross_element fixture exercises this: the NYC migration_in expression references migration_pressure[Boston] and that reference must emit only migration_pressure[boston] -> migration_in[nyc] rather than the full Boston-to-all-targets broadcast. Phase 1's red tests turn green: - element_graph_fixed_index_broadcast_truthful (2N-1 edges, not N^2) - element_graph_wildcard_reducer_plus_bare_truthful (per-ref union) - element_graph_multidim_partial_fixed_conservative - test_cross_element_ltm_edge_set_truthful (no spurious same-element edges from cross-element migration references) The structural flow->stock SameElement bypass and the stock-name expansion pass are preserved unchanged. The lookup_dims closure and dim_cache stay for the same performance reason as before. ElementDependencyKind, classify_element_dependency, classify_in_expr, and expand_edge_to_elements remain temporarily annotated dead_code so the Task 4 commit lands buildable; Task 6 deletes them after Task 5 re-validates the broader simulate_ltm suite.
Delete the obsolete classification machinery now that model_element_causal_edges drives off the AST-walking per-reference emission introduced in the previous commit: - enum ElementDependencyKind - fn classify_element_dependency - fn classify_in_expr - fn expand_edge_to_elements - mod classify_element_dependency_tests The retained helpers (cartesian_element_names, expand_same_element, format_element_name, format_multi_element_name, dimension_element_names) are still consumed by the new walker / emit_edges_for_reference path, so they stay in place.
Four issues from the code review of Phase 2 (AST-walking element graph builder): - M-1: Update module doc to list RefShape and ReferenceSite instead of the deleted ElementDependencyKind enum. - M-2: Remove three stale references to deleted functions classify_in_expr and expand_edge_to_elements in docstrings and inline comments; reword to describe behavior without citing the deleted names. - M-3: Change collect_in_expr parameter target_element from Option<&String> to Option<&str>. At the call site in the Arrayed branch, use target_elem.as_str() instead of allocating a temporary String. Inside the function, change target_element.cloned() to .map(|s| s.to_string()) so the ReferenceSite still gets an owned String. - I-1: Add bare_with_target_element_keeps_only_pinned_diagonal_edge test to emit_edges_for_reference_tests. Exercises the RefShape::Bare + target_element.is_some() path (scratch-map + intersection) on identical single-dim source and target dimensions. Asserts that only pop[boston] -> rel[boston] survives and the off-diagonal pop[nyc] -> rel[nyc] is excluded when the pinned target is "boston".
Add the Phase 3 partial-equation primitives: - classify_expr0_subscript_shape: classifies a parsed-AST subscript's shape (Bare / FixedIndex / Wildcard / DynamicIndex) at the Expr0 level by checking each index against the source's declared dimensions. Mirrors db_analysis::resolve_literal_index but operates before Expr2 lowering so the partial-equation builder can decide per-AST-occurrence whether to leave the reference live or wrap it. - wrap_non_matching_in_previous: walks an Expr0 and wraps each variable reference in PREVIOUS() unless it's the live source whose shape matches the live shape. Indices of subscripts are recursively transformed even when the outer subscript matches the live shape, so nested references in indices are still wrapped. Function names in App nodes are never matched as variables. - build_partial_equation_shaped: the new entry point that parses an equation, computes other_deps (deps minus live_source plus normalize_module_ref filter), and applies the shape-aware transform. Unlike the legacy fast-path build_partial_equation, this version always canonicalizes via parse + print_eqn so downstream parsing receives consistent canonical form. The legacy wrap_deps_in_previous and build_partial_equation remain in place; they are removed at the end of Phase 3 once all callers have migrated. The Phase 1 stub build_partial_equation_for_shape becomes a thin alias delegating to the new shaped builder so the #[ignore]-d Phase 1 tests still compile during this transitional commit; Task 2 removes the alias and migrates the tests directly.
Remove the build_partial_equation_for_shape stub/alias and migrate the four Phase 1 partial-equation tests to call build_partial_equation_shaped directly. The Bare and Wildcard tests still pass an empty source_dim_elements slice -- their classification doesn't depend on element-name lookups because Bare matches any top-level Var of the live source and Wildcard is recognized from the [*] index alone. The FixedIndex tests pass the new region_dim_elements() helper so classify_expr0_subscript_shape can validate [NYC] / [Boston] against the source's declared elements; without that, both literal indices would fall back to DynamicIndex and the live FixedIndex subscript would be incorrectly wrapped. All four formerly-#[ignore]-d Phase 1 tests now pass with the locked expected strings: - Bare: population / sum(PREVIOUS(population[*])) - Wildcard: PREVIOUS(population) / sum(population[*]) - FixedIndex(nyc): (population[nyc] - PREVIOUS(population[boston])) * 0.01 - FixedIndex(boston): (PREVIOUS(population[nyc]) - population[boston]) * 0.01
…ssthrough)
Add two direct unit tests that pin AC2.4 behavior of
build_partial_equation_shaped:
- partial_equation_other_source_always_wrapped: with deps {pop, helper}
and live source pop in shape Bare, the equation 'pop * helper' must
produce a partial that wraps 'helper' in PREVIOUS() while leaving
the 'pop' Var live (no PREVIOUS(pop) anywhere). This exercises the
other_deps path of wrap_non_matching_in_previous.
- partial_equation_unknown_ident_unchanged: with deps {pop} only and
the equation 'pop + unknown', the partial must keep 'unknown'
unwrapped because it isn't in the dep set -- the per-shape builder
treats unknown idents as function-name candidates / external noise
and only wraps known deps. Downstream parsing diagnoses the unknown
separately.
Together with the four Phase 1 tests un-ignored in Task 2, the
partial_equation test set now covers Bare/Wildcard/FixedIndex live-
shape branches plus the AC2.4 invariant that non-live-source deps
are always wrapped.
Add a per-shape link-score naming helper alongside the existing
link-score generators in ltm_augment.rs. The function takes only
(from, to, shape) -- no collision parameter -- so the resulting
name is a stable function of those three inputs across all models.
Naming rules:
- Bare: legacy canonical form ($\u{205A}ltm\u{205A}link_score\u{205A}{from}\u{2192}{to}).
- FixedIndex(elems): per-element prefixed-from form, matching the
existing try_cross_dimensional_link_scores convention.
- Wildcard: ALWAYS suffixed with \u{205A}wildcard.
- DynamicIndex: ALWAYS suffixed with \u{205A}dynamic.
The unconditional Wildcard/DynamicIndex suffixes resolve code-review
issues I2 + I6 from the design plan: with collision-aware naming the
discovery parser would have to reason about per-model coexistence to
recover the canonical (from, to) pair. Always-suffixing makes the
suffix a fixed function of shape, which the discovery parser strips
mechanically (Phase 3 Task 7).
Four unit tests pin the naming rules: Bare canonical, FixedIndex
single-element, Wildcard always suffixed (two examples to make the
cross-model stability explicit), and DynamicIndex always suffixed.
Pivot link-score emission to a per-(from, to, shape) tuple model (Phase 3 Tasks 5 + partial 6). The legacy single-shape path keyed each link score by (from, to) only and emitted one LtmSyntheticVar per causal edge. The new path enumerates the unique RefShapes under which the target's AST references the source and emits one LtmSyntheticVar per shape. Mechanics: - ltm_augment.rs: extend generate_link_score_equation_for_link with shape: &RefShape and source_dim_elements: &[Vec<String>]. The internal aux-to-aux and stock-to-flow generators now route through build_partial_equation_shaped, which holds matching-shape source references live and wraps everything else in PREVIOUS. Flow-to-stock links still use the structural formula and ignore shape. - db_ltm.rs: add link_score_equation_text_shaped(db, link_id, shape, ...) as a salsa-tracked sibling of link_score_equation_text. The legacy function stays for backward compatibility with the existing compile_ltm_var_fragment lookup path. The shape-aware variant computes source_dim_elements via variable_dimensions and threads shape through to generate_link_score_equation_for_link. RefShape derives salsa::Update so it can flow through tracked function args. Lives in db_ltm.rs (alongside the emission loop that calls it) so db.rs stays under the project's per-file line cap. - db.rs (compile_layout): the per-link-score fragment lookup now checks for a shape suffix (wildcard / dynamic) on the to-name in addition to the existing element-subscript check on the from-name. Shape-suffixed and bracketed names route through compile_ltm_equation_fragment directly because the salsa-cached link_score_equation_text path keys on (from, to) and would try to reconstruct the suffixed to as a user variable. - db_analysis.rs: add collect_reference_shapes as a sibling of collect_reference_sites that returns just the unique shapes (no per-site target_element / source name overhead). RefShape is now pub so it can be carried as a field on ltm::Link. - db_ltm.rs (model_ltm_variables): two new helpers drive the per-shape emission. enumerate_shapes reconstructs the target Variable and calls collect_reference_shapes. emit_per_shape_link_scores iterates the shapes and calls link_score_equation_text_shaped per shape, setting the canonical name via link_score_var_name. Both the discovery/sub-model branch and the exhaustive-loop branch flow through this helper. The exhaustive branch falls back to link.shape.unwrap_or(RefShape::Bare) until Phase 4 fills in real shapes. - ltm.rs: add shape: Option<crate::db::RefShape> to the Link struct. All existing constructors initialize shape to None. Phase 4 populates real shape values at loop construction. Two new tests pin the per-shape emission contract: pop->share gets both Bare (pop->share) and Wildcard (pop->share-wildcard) link scores for share = pop / SUM(pop[*]); pop->rel_pop gets both Bare and FixedIndex(nyc) link scores for rel_pop = pop / pop[NYC]. Both tests use a stock for the source because model_ltm_variables short-circuits to an empty result on stockless models. All existing tests (3166 simlin-engine + 48 simulate_ltm + workspace totals) continue to pass: where the target equation references the source under a single shape only, the per-shape emission produces a single Bare-shape link score with the same name as the legacy path.
Update generate_loop_score_equation to look up each link's per-shape
link-score name via link_score_var_name, using link.shape (added in
the previous commit) to disambiguate Bare / FixedIndex / Wildcard /
DynamicIndex variants. Phase 4 will populate Link.shape at loop
construction time; for now, links with shape: None resolve to Bare,
which produces the legacy canonical name and keeps existing loop
scores byte-for-byte identical.
The loop-score equation is the product of per-shape link scores in
the loop. Previously the equation was hand-formatted directly:
'$\u{205A}ltm\u{205A}link_score\u{205A}{from}\u{2192}{to}'. The new
form delegates to link_score_var_name(from, to, &shape) so any future
shape-naming changes only need to update the helper.
A new test (loop_score_equation_references_fixed_index_link_score)
constructs a loop with one explicit FixedIndex(nyc) link and one
unspecified-shape (Bare-by-default) link, then asserts that the
generated equation references both the bracketed FixedIndex name
('pop[nyc]\u{2192}rel_pop') and the Bare canonical name
('rel_pop\u{2192}pop'). Combined with the four link_score_var_name
unit tests from Task 4, the per-shape name resolution is now pinned
end-to-end from link discovery (Phase 4 will provide) through
loop-score equation emission.
Phase 3 introduces two new link score naming patterns that the discovery parser must handle: 1. FixedIndex A2A names like `pop[nyc]→rel_pop` where the source carries a fixed element subscript and the per-slot expansion runs over the target dimension. Each result slot represents the edge for (pop[nyc], rel_pop[d]) at element d. Only the to-name varies per slot; the from-name (pop[nyc]) is reused unchanged. 2. Wildcard / DynamicIndex shape suffixes appended to the to-name (`from→to⁚wildcard`, `from→to⁚dynamic`) to disambiguate per-shape link scores when multiple references with different shapes coexist for the same edge. For offset resolution the suffix is stripped so the canonical to ident matches downstream graph keys. Both paths reuse the cartesian-product expansion factored out into resolve_dim_element_tuples so Bare A2A and FixedIndex A2A stay in lockstep on element ordering.
The pre-Phase-3 single-shape builders are now fully superseded by build_partial_equation_shaped, which holds matching-shape references live and wraps every other reference (including non-matching shapes of the same source) in PREVIOUS(). All production call sites use the shaped variant and the legacy helpers had no remaining callers. Also drops the stale #[allow(dead_code)] annotations on classify_expr0_subscript_shape, wrap_non_matching_in_previous, wrap_index_non_matching_in_previous, and link_score_var_name -- those functions are wired up to real callers now -- and refreshes the ltm_augment.rs entry in src/simlin-engine/CLAUDE.md to describe the shape-aware API surface instead of the deleted legacy one.
…k_scores The match on lsv.dimensions had two arms (RefShape::FixedIndex(_) and _) that both returned target_dims.clone(). The arms were identical so the match added no control-flow value. Replace with a single direct assignment and consolidate the two explanatory comments into one so the FixedIndex semantics documentation is retained.
Add infer_link_shape_from_node_name and parse_subscript helpers in db_ltm.rs alongside strip_subscript. Phase 4's loop-link annotation needs to translate element-level node names like 'pop[nyc]' or 'share[boston,adult]' back into the RefShape values that downstream link-score selection consumes. The helpers are pub(crate) so the follow-on annotation pass in build_element_level_loops can call them, and the fallback for malformed bracket sequences is to treat the name as bare so the caller degrades to the safe default rather than emitting a malformed FixedIndex shape. The dead_code allow lifts only until Phase 4 Task 3 wires the helpers into build_element_level_loops; the unit tests cover the helpers directly in the meantime.
|
@codex review |
generate_loop_score_equation always called link_score_var_name with RefShape::Bare, but emit_per_shape_link_scores only produces the names that actually appear in the target's AST. For a fixture like share[r] = SUM(pop[*]) the only emitted name for the (pop, share) edge is pop->share:wildcard; the loop_score equation referenced pop->share (Bare canonical), which was never emitted, so the fragment compiler silently inserted a stub dep and dropped that link's contribution to the loop score. Fix: thread the set of emitted link-score names down through generate_loop_score_variables to generate_loop_score_equation, and resolve each loop link against that set with shape priority Bare > Wildcard > DynamicIndex. Cross-dimensional edges keep working because their bracketed link.from combined with Bare naming matches what try_cross_dimensional_link_scores produces. The resolver falls back to the Bare canonical name when no candidate is in the emitted set, matching the prior unreachable-branch behavior. Two regression tests pin this: a unit test on generate_loop_score_equation that asserts the wildcard suffix wins when Bare is absent from emitted, and an integration test on the wildcard-only feedback fixture (share[r] = SUM(pop[*]) feeding back through update[r] -> pop[r]) that asserts every loop_score equation reference resolves to an emitted variable.
|
@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: 34835efb3d
ℹ️ 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".
| let shapes = enumerate_shapes(db, source_vars, from, to, model, project) | ||
| .filter(|s| !s.is_empty()) | ||
| .unwrap_or_else(|| vec![fallback_shape]); |
There was a problem hiding this comment.
emit_per_shape_link_scores now emits only the shapes found in the AST, so edges that are wildcard/fixed-only no longer produce the canonical bare name. However, pathway/composite generation in model_ltm_variables still hardcodes "$⁚ltm⁚link_score⁚{from}→{to}", so input-port models with edges like SUM(x[*]) can reference link-score vars that were never emitted, causing path/composite scores to compile against stub deps (effectively dropping those pathways). Please either emit a bare alias here or add the same emitted-name resolution used for loop scores.
Useful? React with 👍 / 👎.
| for shape in [RefShape::Bare, RefShape::Wildcard, RefShape::DynamicIndex] { | ||
| let candidate = link_score_var_name(from, to, &shape); | ||
| if emitted.contains(&candidate) { |
There was a problem hiding this comment.
Loop-score name resolution only checks Bare/Wildcard/Dynamic shapes, so loops that traverse an edge emitted only as FixedIndex (for example a feedback edge from pop[nyc] in share[r]=pop[nyc]) fall back to a non-existent bare name. In that case the loop-score equation multiplies a missing link-score variable and silently loses that loop contribution. The resolver should consider FixedIndex-emitted names (or preserve the fixed-index form on loop links) before falling back.
Useful? React with 👍 / 👎.
Code review[P1]
|
Two related Codex P1 findings, addressed by extending the same resolver
introduced for loop_score and applying it to pathway/composite emission.
P1 #A (FixedIndex-only edges in loops): the loop-score resolver only
checked Bare/Wildcard/Dynamic. For models like share[r] = pop[NYC] the
only emitted name for (pop, share) is pop[nyc]->share (FixedIndex via
the bracketed-from prefix). The loop-score equation referenced the
Bare canonical pop->share, which was never emitted, so the fragment
compiler silently inserted a stub dep.
P1 #B (pathway/composite hardcoded Bare canonical): pathway emission
in model_ltm_variables hardcoded format!("...link_score:{from}->{to}")
instead of resolving against emitted names. Same failure mode for
sub-models with input ports whose paths traverse wildcard/fixed-only
edges (e.g., summed = SUM(x[*]) inside a sub-model with input port x).
Fix:
1. Extend resolve_link_score_name_for_loop with a FixedIndex priority
level. Between Bare (priority 1) and Wildcard (priority 3), the
resolver now scans the emitted set for any name matching
{from}[...]->{to} (no shape suffix) and picks the lexicographically
first match. This deterministically resolves multi-element targets
like share[r] = pop[NYC] + pop[BOSTON] -- picking one is the
documented edge-aliasing under-counting limitation, now manifesting
across multiple FixedIndex variants instead of across shapes.
2. Apply the same resolver in the pathway/composite emission loop
in db_ltm.rs. Pre-build the emitted-link-score-names HashSet once
before the pathways loop and pass it to the resolver per link.
Coverage:
- Four new resolver unit tests pin the priority order: FixedIndex
when Bare absent, lexicographic determinism with multiple FixedIndex
variants, Bare wins over coexisting variants, and bracketed-from
cross-dim case still resolves via Bare naming.
- Existing loop_score integration test exercises the same resolver
end-to-end through a different call path.
- Existing pathway/composite tests pass unchanged, verifying no
regression on the common Bare-only case.
- The mechanical pathway fix (hardcoded format -> resolver call) is
verified by code inspection plus the resolver unit tests.
Constructing an arrayed sub-module fixture for an end-to-end pathway
integration test proved fiddly enough to risk drift; deferred in favor
of relying on the unit-test coverage above.
|
@codex review |
Review findings[P3] Expr0
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cec0713c6
ℹ️ 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".
| // the synthesized variable name to disambiguate per- | ||
| // shape link scores; for offset resolution the | ||
| // canonical to ident is suffix-free. | ||
| let to_str = strip_to_shape_suffix(to_raw); |
There was a problem hiding this comment.
Stripping the to-side shape suffix here collapses distinct per-shape link scores onto the same (from,to) key (for example, pop→share and pop→share⁚wildcard from share[r] = pop / SUM(pop[*])), so parse_link_offsets can emit duplicate edges for one pair. Downstream, discover_loops_with_graph materializes a HashMap<(from,to), offset> and whichever duplicate is inserted last wins, which depends on results.offsets hash iteration order; this makes discovery pick an arbitrary shape-specific series and can produce nondeterministic loop/pathway scores across runs.
Useful? React with 👍 / 👎.
|
@codex review |
Codex P1 (latest round): when emit_per_shape_link_scores produces both pop->share (Bare) and pop->share:wildcard (Wildcard) for the same edge (e.g., share[r] = pop / SUM(pop[*])), parse_link_offsets stripped the shape suffix to recover the canonical (from, to) key -- collapsing both variants onto the same key with two different offsets. Two downstream effects, both nondeterministic: - SearchGraph::from_results pushed each LinkOffset into the adjacency list, registering two parallel edges from pop to share with different scores. After sort-by-abs-descending, the strongest-path search picked whichever happened to have the higher abs(score). - discover_loops_with_graph::link_offset_map collapsed the duplicates by HashMap insert order over results.offsets -- nondeterministic iteration order. The graph search and the per-loop scoring could even land on different variants for the same edge. Also affects A2A: pop->births:wildcard with non-empty dims expands to per-element keys (pop[nyc], births[nyc]) etc., colliding with Bare A2A's expansion of pop->births at the per-element level. Fix: tag each parsed entry with its shape rank (Bare > Wildcard > DynamicIndex), then dedupe by (from, to) keeping the lowest-rank entry. Sort the result for cross-run determinism. The priority order mirrors ltm_augment::resolve_link_score_name_for_loop so loop_score, pathway, and discovery all reference the same shape variant for any given edge -- the documented edge-aliasing under-counting limitation is consistent across consumers. FixedIndex names like pop[nyc]->share produce element-level keys that don't collide with Bare's (pop, share) key, so they don't participate in dedup; tagged at the Bare rank for simplicity (the dedup is a no-op for them). Two regression tests cover the scalar and A2A collision paths. Both go RED on the bug and GREEN on the fix.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efd8ec5918
ℹ️ 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".
| if rank < existing.1 { | ||
| *existing = (offset, rank); |
There was a problem hiding this comment.
When both a Bare A2A link score and a FixedIndex A2A link score map to the same expanded key (for example, pop→share and pop[nyc]→share both yielding (pop[nyc], share[nyc])), both entries carry ShapeRank::Bare, so this update path never runs and the survivor depends on results.offsets hash iteration order. That makes discovery edge weights non-deterministic across runs and can silently switch which series is used for the same (from,to) edge.
Useful? React with 👍 / 👎.
Code review[P2]
|
Codex P1 (latest round): when both Bare A2A (pop->share) and FixedIndex A2A (pop[nyc]->share) emit for the same edge -- e.g., share[Region] = pop + pop[NYC] -- both expansions produce the per-element key (pop[nyc], share[nyc]). My previous fix tagged FixedIndex names with the Bare rank "for simplicity", so the dedup saw a tie at Bare and the survivor depended on HashMap insertion order over results.offsets. Same nondeterministic discovery weights the suffix-collision fix was meant to eliminate. Two related corrections: 1. ShapeRank now has a distinct FixedIndex variant between Bare and Wildcard. Classification picks suffix first (Wildcard / Dynamic when present), then `[`-in-from (FixedIndex), else Bare. Suffix wins over from-side bracket because Wildcard/Dynamic and FixedIndex are mutually exclusive in the design (link_score_var_name never combines them); the explicit precedence makes the rank deterministic even if a future emitter mixed them. 2. Dedup compares (rank, offset) tuples, breaking same-rank ties on the smaller offset. With distinct ranks this matters only as a defensive belt-and-suspenders against future emitter changes that might produce two same-rank entries for one expanded key, but it eliminates any remaining HashMap-iteration-order dependence. The priority order Bare > FixedIndex > Wildcard > DynamicIndex matches resolve_link_score_name_for_loop, so loop_score, pathway, and discovery all land on the same shape variant for any given edge -- the documented edge-aliasing under-counting limitation stays consistent across consumers. New regression test exercises the Bare A2A + FixedIndex A2A collision; goes RED on the bug (offset 20 wins) and GREEN after (offset 10 wins). The non-aliased FixedIndex slot (pop[nyc], share[boston]) -- which Bare A2A doesn't produce -- correctly survives at FixedIndex's offset.
|
@codex review |
Code review[P3] Integer-literal bounds asymmetry between Expr2 and Expr0 subscript classifiers
Misleading "Phase 4" comments in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29639bf180
ℹ️ 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".
| let partial_eq = | ||
| build_partial_equation_shaped(&to_equation, &deps, from, shape, source_dim_elements); | ||
|
|
||
| let from_q = quote_ident(from.as_str()); |
There was a problem hiding this comment.
generate_auxiliary_to_auxiliary_equation now builds a shape-specific partial equation, but the denominator still uses unsuffixed from (from_q), so FixedIndex scores are normalized by the current element instead of the referenced element. For a link like rel_pop[r] = pop / pop[NYC], the emitted $⁚ltm⁚link_score⁚pop[nyc]→rel_pop series at r=boston is divided by Δpop[boston] instead of Δpop[nyc], which distorts magnitude/sign and can collapse valid cross-element influence to zero. The denominator should be derived from the same shape-specific source reference used to keep the live term in partial_eq.
Useful? React with 👍 / 👎.
|
@codex review |
Three findings from the latest review round:
Codex P1 (FixedIndex link-score denominator): the SAFEDIV denominator
that drives the link-score SIGN used the unsuffixed `from` for every
shape, so a FixedIndex link score `pop[nyc]->rel_pop` divided by Δpop
(per-element under A2A: Δpop[r]) instead of Δpop[nyc]. Per-element
targets at r != nyc would normalize by an unrelated source delta,
distorting magnitude/sign and potentially zeroing valid cross-element
influence whenever pop[nyc] and pop[r] move in opposite directions.
Fix: extract `shape_aware_source_ref(from, shape)` -- returns
`from[elem]` (subscript syntax, not a quoted ident) for FixedIndex,
and the bare `from` for other shapes. The early-return zero-guard
uses the same reference. Bare and Wildcard/DynamicIndex are
unchanged: the truly principled denominator for Wildcard would be
the change in the aggregate referenced live in `partial_eq` (e.g.,
Δ(SUM(from[*]))), but threading the AST subtree through is a larger
refactor; the current variable-level Δfrom is the same documented
edge-aliasing limitation already pinned by integration tests. Both
the auxiliary-to-auxiliary and stock-to-flow generators get the fix.
Claude P3 (integer-literal bounds asymmetry): the Expr2 classifier
in db_analysis::resolve_literal_index validates integer literals
against the indexed dim's size and returns None (so the shape
becomes DynamicIndex) for out-of-range values, while the Expr0
classifier in ltm_augment accepted any u32-parseable Const without
bounds checking. They could disagree on `pop[999]` over an
indexed dim of size 5: edge emitter -> DynamicIndex; partial-equation
builder -> FixedIndex(["999"]). The shape mismatch wraps the live
reference in PREVIOUS, silently zeroing the link score.
Fix: extract `resolve_literal_element_index` -- the Expr0 sibling
of resolve_literal_index. Both `is_literal_element_index` and
`classify_expr0_subscript_shape` route through it. Integer literals
are now validated by membership in `source_dim_elements` (which
contains `["1", ..., "size"]` for indexed dims), so out-of-range
integers fall through to `DynamicIndex` -- matching what the edge
emitter sees.
Claude P3 (stale "Phase 4" comments): every `Link { from, to,
polarity }` literal carried a `// Shape is populated in Phase 4 at
loop construction.` comment. The shape field was removed when we
encoded the per-reference shape in the link.from / link.to strings;
the comments dangled. Removed all 27 occurrences across ltm.rs,
ltm_finding.rs, and libsimlin/src/model.rs.
Two regression tests:
- `fixed_index_link_score_denominator_uses_fixed_element` asserts
the FixedIndex link score's denominator references `pop[nyc]`
(not the unsuffixed `pop`), and the Bare variant keeps its
unsuffixed `Δpop` denominator.
- `classify_expr0_rejects_out_of_range_integer_literal` asserts
`Const("999", ...)` over an indexed dim of size 2 classifies as
`DynamicIndex`, with sanity check that an in-range integer still
classifies as `FixedIndex`.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44ce84c03c
ℹ️ 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".
| if let Ok(n) = canonical.parse::<u32>() | ||
| && n >= 1 | ||
| && n <= *size | ||
| { | ||
| return Some(canonical); | ||
| } |
There was a problem hiding this comment.
In resolve_literal_index, indexed-dimension literals are accepted by numeric range but returned with their original text, so x[01] becomes RefShape::FixedIndex(["01"]). Downstream code assumes canonical element names ("1", "2", ...): the Expr0 classifier in ltm_augment treats 01 as non-literal, and element-node naming uses canonical indices, so shape matching can fail and the live reference may get wrapped in PREVIOUS() (or mapped to a non-existent ...[01] node). This only appears when users write non-canonical integer subscripts (e.g., zero-padded), but then link scores/element edges are silently miscomputed.
Useful? React with 👍 / 👎.
Code reviewI focused on the new per-reference element-graph walker ( Findings[P2] Cross-element approximation loops violate the new
The cross-element branch in Overall correctnessCorrect, with the caveat above: the docstring's stated invariant is not enforced for cross-element approximation loops, so models with cross-element feedback through stocks may get silently wrong relative loop scores. This is preexisting behavior, but the docstring being introduced in this PR makes the gap newly load-bearing. |
… cross-element loops Two P2 findings, both real correctness bugs at the silent-zero / silent-misnormalization level: Codex P2 (integer-literal canonicalization). After the previous round unified the Expr0 and Expr2 classifiers via a shared validator, `db_analysis::resolve_literal_index` still returned the original subscript text for indexed-dim integer literals -- so `pop[01]` over an indexed dim of size 5 became `RefShape::FixedIndex(["01"])`. The Expr0 sibling and `dimension_element_names` both speak in canonical "1".."N" form, so the partial-equation builder reclassified the same reference and the shape comparison failed -- live ref wrapped in PREVIOUS, link score silently zero. Fix: canonicalize via parse-then-format in BOTH classifiers (`n.to_string()`), so `pop[01]` reduces to `FixedIndex(["1"])` everywhere. Claude P2 (cross-element stocks granularity). The `Loop` docstring's stocks-granularity invariant says any loop with `dimensions.is_empty()` MUST carry element-level stock names so `partition_for_loop` can resolve them in `model_element_cycle_partitions::stock_partition` (element-level keys). The mixed/scalar branch in `build_element_level_loops` follows this rule (cycle 3 fix), but the cross-element branch was using variable-level stocks via `var_graph.find_stocks_in_loop(&unique_cycle)`. Cross-element loops have `dimensions: vec![]`, so by the docstring they should be element-level too -- but they weren't. Effect: every cross-element loop with stock nodes (e.g., the population stocks in `cross_element_ltm`, or any model with wildcard- reducer feedback through arrayed stocks) returned `None` from `partition_for_loop` and bucketed into the catch-all None group in `compute_rel_loop_scores`, getting normalized against unrelated loops instead of within its own SCC. Fix: collect element-level stock nodes from the original element- level circuit (mirroring the mixed/scalar branch's pattern). A single cross-element loop typically traverses the same stock variable at multiple elements (population[nyc] AND population[boston]); include all of them so the partition lookup hits the SCC containing all participating elements. Also tightens the `Loop` docstring to make the cross-element inclusion explicit, removing the gap Claude flagged. Two RED-then-GREEN regression tests: - `classify_expr0_canonicalizes_integer_literal_subscript` -- asserts `pop[01]` over indexed-size-5 dim classifies as `FixedIndex(["1"])`, matching the canonicalized Expr2 output. - `cross_element_loop_partitions_resolve_to_some` -- builds the wildcard-reducer cross-element fixture, finds every loop with empty `dimensions` (cross-element / scalar / mixed), and asserts each resolves to a Some partition. A2A loops legitimately return None (they don't use element-level partition lookup) and are correctly excluded from the assertion.
|
@codex review |
Review summaryI reviewed the per-reference element graph and per-shape link score refactor end-to-end. The code is generally well-structured with strong test coverage of the core invariants:
A couple of minor observations (no specific findings to file):
Overall correctnessThe patch looks correct. The refactor preserves the variable-level projection invariant, the per-shape semantics are tested at both the unit level ( |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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". |
1 participant
Summary
ElementDependencyKindclassification with a per-AST-reference walker (collect_reference_sites+emit_edges_for_reference). Fixed-index references likepopulation[NYC]now produce truthful 2N-1 edges instead of N² spurious all-pairs.RefShapeenum (Bare / FixedIndex / Wildcard / DynamicIndex) and per-shape partial equations + link scores viabuild_partial_equation_shapedandlink_score_var_name. Wildcard/DynamicIndex link scores carry stable suffixes (⁚wildcard,⁚dynamic) so names are independent of which other shapes coexist.MAX_LTM_SCC_NODES = 50retained — measurements show no fixture moved closer to the threshold.Implementation phases
7 phases over 53 commits, each with code review (some after multiple cycles):
ElementDependencyKind)Link.shape: Option<RefShape>threaded; discovery parser updated)build_element_level_loops(Critical bugs surfaced and fixed;Loop.stocksgranularity invariant enforced via debug_assert)Test plan
cargo test --workspacepasses (verified locally throughout)docs/test-plans/2026-04-25-ltm-per-ref-elem-graph.md— fixtures:cross_element_ltm,arrayed_population_ltm, WRLD3