Repository navigation
engine: surface polarity confidence for LTM loop classification - #490
Conversation
Review[P3]
|
bpowers
left a comment
bpowers
left a comment
There was a problem hiding this comment.
Adversarial review of PR #490 -- LTM polarity confidence
Verification snapshot:
cargo build -p simlin-engine-- cleancargo test -p simlin-engine --features file_io-- all suites pass; newpolarity_confidence_tests(5) andfrom_runtime_scoresunit tests (10 total in ltm.rs) all green; per-test runtime well under 1scargo clippy --workspace --all-targets -- -D warnings-- cleancargo fmt --check-- cleancargo test -p simlin(libsimlin) -- 20/20 pass
Citations spot-checked. Formula and threshold cited from docs/reference/ltm--loops-that-matter.md section 13.7 are correct (|r - |b|| / (r + |b|), "0.99 or lower triggers gray"). Thesis summary section 7.6 (Rux/Bux/Ux labelling) is also correct. Plan alignment with #485 is excellent: the four bullets in the issue are all addressed, no scope creep, single-commit PR.
Overall: solid work. Strong rustdoc, well-targeted tests, principled choice to coalesce mostly-R/B at the FFI/metadata boundaries with explanatory comments at every site. The bulk of issues below are about consequences in adjacent surfaces (pysimlin, MCP docs) rather than the core algorithm.
Decision: COMMENT-only. Two Important items below should land as small follow-ups (or in this PR if you prefer), but none of them are correctness blockers for the engine work.
Critical
None.
Important
I-1. pysimlin Python LoopPolarity.from_runtime_scores is now divergent
src/pysimlin/simlin/analysis.py:48-81 reimplements from_runtime_scores in Python with the old semantics: any sign change collapses to UNDETERMINED, no confidence ratio, no Mostly-R/B variants. After this PR the Python and Rust implementations of the "same" function disagree:
- For input
[1, 2, 3, 0.5, -0.01]Rust saysMostlyReinforcing(conf=0.997); Python saysUNDETERMINED.
Call site src/pysimlin/simlin/run.py:255 then uses Python's classification to override the structural polarity that came in over the FFI. Because the FFI deliberately coalesces mostly-R/B back to R/B, the practical user-visible damage is bounded -- but the two from_runtime_scores definitions are now meaningfully out of sync, and tests/test_ltm_polarity.py doesn't cover the mostly-R/B case so the divergence is silent.
Two reasonable fixes:
- Port the new logic + threshold + variants into the Python enum and update
run.py:255to consume the(polarity, confidence)tuple. Add the missing test cases. - Or, delete the Python implementation entirely and route through the C ABI (forces a small FFI broaden, but eliminates the duplicate forever).
Either way, please track the divergence -- right now the Python "Loops that Matter" view will silently misclassify Rux/Bux loops on Python-side inputs.
I-2. MCP instructions.md undersells the new polarity strings
src/simlin-mcp/src/instructions.md:82 documents polarity as exactly "reinforcing", "balancing", or "undetermined". With to_loop_summary now emitting "mostly_reinforcing" and "mostly_balancing" (src/simlin-engine/src/analysis.rs:271-274), AI agents driving the MCP tool will encounter polarity strings the docs claim are impossible. Worth a one-line addition listing all five, ideally with a short note that mostly-R/B means "predominantly R/B at runtime, confidence at or above 0.99".
simlin-mcp-core/src/types.rs::LoopDominanceSummary.polarity: String passes through verbatim, so the wire surface itself is fine -- this is purely a docs gap.
Minor
M-1. Author order in citations is inverted
The paper "Seamlessly Integrating Loops That Matter into Model Development and Analysis" is by Schoenberg and Eberlein (2020) (paper #4 in docs/reference/ltm--loops-that-matter.md references), not "Eberlein & Schoenberg" as repeated throughout src/simlin-engine/src/ltm.rs:158, 195, 216, 283 and the PR description. Paper #3 by Eberlein & Schoenberg is "Finding the Loops that Matter" -- a different paper. Recommend swapping author order in the rustdoc citations and PR body, and ideally including the paper title to disambiguate from #3.
M-2. >= vs strictly > 0.99 threshold disagreement between sources
The implementation uses confidence >= POLARITY_CONFIDENCE_THRESHOLD (line 259 of ltm.rs). The cited LTM doc section 13.7 says "0.99 or lower triggers gray" -- so < 0.99 is gray, >= 0.99 is opaque -- which matches the code. But the thesis summary section 7.6 explicitly says Rux/Bux: confidence > 0.99. The two reference docs disagree by a hair. In practice ratios landing exactly on 0.99 are vanishingly rare in floats, but worth a one-line comment in ltm.rs near line 259 acknowledging that the two sources differ at the boundary and that the implementation follows section 13.7's "0.99 or lower triggers gray" reading.
M-3. Confidence aggregation interpretation
Doc 13.7 defines r = "sum of the single highest magnitude instantaneous reinforcing pathway scores across the entire simulation" (and similarly b). That's an across-pathway max per timestep, summed across time. The implementation applies the same formula to a single loop's loop_score series over time, which is a one-pathway-per-timestep specialisation -- a reasonable mapping, but not literally what 13.7 describes. A two-line comment in the rustdoc above from_runtime_scores mapping the per-link/per-pathway formula in 13.7 onto the per-loop-score-series specialisation here would make the equivalence explicit and protect against future "this doesn't match the doc" reports.
M-4. tech-debt entry #30 marked RESOLVED before the PR merges
docs/tech-debt.md:274 now says **Severity**: RESOLVED (2026-05-06). Convention elsewhere in the file is to mark RESOLVED with the merge commit hash after merge, not the branch state. If the PR sits in draft for a while or is rebased away, the doc on main (after eventually merging) will say RESOLVED with no commit anchor. Two safe options: (a) revert the docs change in this PR and apply it as a tiny follow-up commit after merge; or (b) keep the change but add a "(pending merge of #490; commit will be backfilled)" caveat. The text otherwise looks great -- accurate description of the new behaviour.
M-5. Web/TS layer silently sees coalesced polarity
This is the explicit trade-off the implementer flagged at FFI / metadata / layout boundaries. Worth surfacing once at PR-summary level: until libsimlin grows a confidence field, every WASM/CGo/C/web-UI consumer sees Reinforcing for what the engine internally classifies as MostlyReinforcing. JSON output through analysis::to_loop_summary is the only path that surfaces the distinction today. That is the right call for binary compat, but it's worth a tracked follow-up issue: "expand SimlinLoop to carry polarity_confidence and add MostlyReinforcing/MostlyBalancing variants behind an opt-in" so this trade-off isn't permanent.
M-6. simulate_ltm.rs:655-658 integration assertion no longer covers the new variants
tests/simulate_ltm.rs:656-659 checks polarity == Reinforcing || polarity == Balancing for SMOOTH-containing loops. With the new variants this still passes (the structural pipeline never emits MostlyReinforcing/MostlyBalancing today), but the assertion is now a partial match. Tiny improvement: include the mostly-R/B variants in the OR so the test stays exhaustive if/when structural classification ever overlays runtime evidence.
Praise
- Rustdoc on
LoopPolarity,POLARITY_CONFIDENCE_THRESHOLD, andfrom_runtime_scoresis unusually thorough and references the relevant doc sections with precise anchor text. This is the documentation we want. - The single-pass running-sum implementation in
from_runtime_scoresis cleaner than the original allocatingVec<f64>. Net reduction of allocations and gain of new functionality. - Every coalesce site (libsimlin, layout, analysis::to_feedback_loop) carries a comment explaining why the mostly-R/B variants collapse to R/B at that boundary, naming what would have to grow a confidence field for the collapse to go away. Future maintainers won't "fix" this.
- The
(false, false) => unreachable!()arm with the surrounding invariant comment is exactly right: encode the invariant in code, document why it holds, let the compiler enforce exhaustiveness. - Comment at lines 263-267 of ltm.rs precisely explains why the magnitude-tie case is unreachable. Saves a future reader an investigation.
- New test
test_loop_polarity_from_runtime_scores_mostly_reinforcinguses an explicit math comment showing the expected ratio derivation -- excellent. polarity_confidence_tests::structural_undetermined_loop_has_zero_confidencecorrectly drives the U classification through a non-monotone graphical function rather than fabricating a degenerate model. Tests the right code path.small_simulated_model_surfaces_consistent_confidenceis a tight one-stock-one-loop end-to-end test that pins the contract that structural and runtime confidence agree at 1.0 for unambiguous loops, exactly as the test docstring claims. Runs in well under a second.- Single, self-contained commit. No scope creep. No emoji, no Co-Authored-By. Commit message is appropriately detailed.
Three small fixes from the PR #490 review: - The "Seamlessly Integrating LTM" paper that introduces the 0.99 confidence cutoff is by Schoenberg & Eberlein, not Eberlein & Schoenberg (Article #5 in the thesis); flip the citation order in every comment that references that paper. The "Finding the Loops that Matter" paper cited in ltm_finding.rs *is* Eberlein & Schoenberg, so leave that one alone. - Document the `>=` vs `>` ambiguity around POLARITY_CONFIDENCE_THRESHOLD: the 2020 paper text says "above 0.99" while section 13.7's gray rendering rule says "0.99 or lower," leaving the boundary case ambiguous. This implementation uses the inclusive form to match the 13.7 formula, captured in a new comment so future readers know it was a deliberate choice. - Make the simulate_ltm structural-polarity assertion exhaustive over DetectedLoopPolarity instead of `==` against R/B only. Mostly_* values would slip through the disjunction silently; matching the whole enum surfaces them as a regression at the test site (the structural pipeline currently never emits Rux/Bux, so the test remains green).
Convention is to mark a tech-debt entry RESOLVED only after the PR that fixes it has merged, with the resolving commit hash captured in the description. PR #490 is still in draft, so flip entry #30 back to "low" severity and note the in-flight branch instead. The entry will be marked RESOLVED post-merge along with the merge SHA.
bpowers
left a comment
bpowers
left a comment
There was a problem hiding this comment.
Review feedback addressed
I-1 (pysimlin divergence): fixed in 1f646c6
I-2 (MCP instructions): fixed in 2acb78f
M-1 (paper author order): fixed in 3f74ac6
M-2 (>=0.99 ambiguity): fixed in 3f74ac6
M-3 (tech-debt #30 reverted): fixed in 80e463a
M-4 (exhaustive simulate_ltm match): fixed in 3f74ac6
M-5 (TS/JSON Rux/Bux follow-up): filed as #495
PR remains in draft.
Findings[P3]
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #490 +/- ##
==========================================
+ Coverage 82.39% 82.42% +0.02%
==========================================
Files 243 243
Lines 61533 61822 +289
==========================================
+ Hits 50701 50956 +255
- Misses 10832 10866 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@codex review |
bpowers
left a comment
bpowers
left a comment
There was a problem hiding this comment.
Latest review feedback addressed
P3 (Inf-handling in from_runtime_scores): fixed in 70b17f9. Filter tightened from is_nan() || v == 0.0 to !v.is_finite() || v == 0.0 so Inf/-Inf no longer poison denom into producing Inf / Inf = NaN confidence. Added unit tests for all-Inf, +Inf-with-finite-reinforcing, and -Inf-with-finite-balancing.
P3 (loop-dominance.md skill missing MOSTLY_*): fixed in 50c07a4. Skill now enumerates all five LoopPolarity variants and notes that Run.loops can return any of them while Model.loops (structural-only) reports just the legacy three.
Branch also picked up 8d15b7e (cherry-picked from ltm/480-array-reducer-polarity) so the workspace pre-commit hook completes; that test recursion was unrelated to this PR's content.
PR is now marked ready for review.
[P2] Python
|
![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: 50c07a4eae
ℹ️ 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 pol == LoopPolarity.REINFORCING or pol == LoopPolarity.MOSTLY_REINFORCING: | ||
| reinforcing_loops.append((lid, score)) | ||
| elif pol == LoopPolarity.BALANCING: | ||
| elif pol == LoopPolarity.BALANCING or pol == LoopPolarity.MOSTLY_BALANCING: | ||
| balancing_loops.append((lid, score)) |
There was a problem hiding this comment.
_calculate_dominant_periods now buckets MOSTLY_REINFORCING and MOSTLY_BALANCING as fixed reinforcing/balancing classes, but these loops are explicitly mixed-sign over time; when a mostly-reinforcing loop has a negative score at a timestep, this code still adds its abs(score) to the reinforcing side, which can flip the winning polarity and yield incorrect dominant-period boundaries/loop sets. This is a regression from prior behavior where mixed-sign loops were treated like undetermined and assigned by timestep sign, and it can corrupt loop-dominance results in nonlinear runs with sign-switching loops.
Useful? React with 👍 / 👎.
Plans the tiered enumeration that runs Johnson on the variable graph first, classifies each cycle by RefShape composition, and emits pure-A2A and pure-scalar cycles as Loops directly without entering the element-level enumerator. Only mixed and cross-element cycles flow through element-level Johnson, and only on the subgraph induced by their nodes -- not the full element graph. Cost becomes additive in N for pure-A2A loops where today it is multiplicative; the auto-flip gate's domain narrows to the cross-element / mixed slice. The full assessment, fixture predictions, and stage breakdown are in the design plan.
Adds `model_edge_shapes`, a salsa-tracked function that classifies every
variable-level edge `(from, to)` by the set of `RefShape` values seen at
any AST reference site of `from` in `to`'s equation. The result is the
input the upcoming tiered loop enumerator needs to decide whether each
variable-level cycle is pure-A2A (every edge has only Bare references),
pure-scalar, or cross-element / mixed (any Wildcard, DynamicIndex, or
FixedIndex shape).
Structural flow->stock edges have no AST reference, so they map to
`{Bare}` directly -- semantically a same-element diagonal, matching
the classification `model_element_causal_edges` already uses for the
same case. Module input edges and unable-to-reconstruct fallbacks
likewise map to `{Bare}` (modules are scalar nodes; the fallback is
defensive).
`RefShape` gains a derived `Ord` so the result type can use
`BTreeSet<RefShape>` for deterministic iteration. The walker reuses
the existing `collect_reference_shapes` helper; the per-target AST
parse results are already cached by salsa, so the second walk that
`model_element_causal_edges` performs is cheap.
This is Stage 1 of the variable-level loop enumeration plan
(docs/design-plans/2026-05-06-ltm-482-variable-level-loop-enumeration.md);
behavior on existing fixtures is unchanged because the new function
is not yet consumed by any downstream code path.
Adds `model_loop_circuits_tiered`, the salsa-tracked tiered loop enumerator that runs Johnson on the variable graph first and only descends to element-level Johnson for cycles that actually need it. The classifier (`classify_cycle`) labels each variable-level cycle as PureScalar, PureSameElementA2A, or CrossElementOrMixed by reading the per-edge RefShape map from `model_edge_shapes`. Pure-scalar and pure-A2A cycles materialize directly into a single Loop without entering element-level enumeration; only mixed/cross-element cycles flow through Johnson on the slow-path subgraph induced by their nodes. The slow-path subgraph is a strict subset of the full element graph on any model where pure cycles exist. For purely scalar models (e.g. WRLD3) and purely A2A models (e.g. arrayed_population_ltm), the slow-path subgraph is empty and zero element-level circuits are enumerated. The cost asymmetry that made pure-A2A cycles inflate multiplicatively in N is gone. The new function is salsa-tracked separately from `model_element_loop_circuits`; the latter is unchanged because no caller has been wired to the tiered representation yet. Stage 4 will update `build_element_level_loops` in db_ltm to consume the tiered result and emit Loops directly for fast-path cycles.
Wires `model_ltm_variables` to consume `model_loop_circuits_tiered` via a new `build_loops_from_tiered` helper. Pure-A2A and pure-scalar cycles are now emitted directly as Loops without paying for element-level Johnson; only cross-element / mixed cycles flow through the legacy element-grouping logic on the slow-path subgraph. The legacy `build_element_level_loops` helper is preserved and is still called by `build_loops_from_tiered` to handle slow-path element circuits. The merged Loop list (fast-path + slow-path) gets one final `assign_loop_ids` pass so loop ID numbering is consistent across both paths. Existing fixtures continue to pass: `assign_loop_ids` sorts loops by content-derived key (sorted distinct variable names) before numbering, so r1/b1/u1 IDs are stable regardless of which path produced each Loop. The 48 `tests/simulate_ltm.rs` integration tests pass without modification. Stage 4 of `docs/design-plans/2026-05-06-ltm-482-variable-level-loop-enumeration.md`.
… graph
Restructures the LTM auto-flip gate from a single check on the full
element-level graph SCC into two phased checks:
1. Variable-level SCC (cheap Tarjan on the variable graph). If
this exceeds MAX_LTM_SCC_NODES, auto-flip immediately -- no
variable-level Johnson runs.
2. Slow-path subgraph SCC (computed inside
model_loop_circuits_tiered). When the cross-element / mixed
subgraph's SCC exceeds the threshold, the tiered enumerator
skips Johnson on that subgraph and emits an empty slow_path;
the caller flips to discovery mode and emits a diagnostic.
After the per-reference element graph refactor (#448), pure-A2A
models already have N small element-level SCCs (one per element),
so the legacy gate didn't actually trip on them. The structural
benefit of the new gate is that gate cost is now proportional to
the variable graph and slow-path subgraph rather than the full
N-times-inflated element graph, and TieredCircuitsResult exposes
slow_path_largest_scc so the gate logic is local and explicit.
The headline performance win for #482 is the elimination of
redundant per-element circuit enumeration on pure-A2A loops:
`tests::pure_a2a_eliminates_per_element_circuit_redundancy`
asserts that the legacy element-level Johnson enumerates 30
circuits on a 30-element pure-A2A model while the tiered
enumerator emits one fast-path cycle and zero slow-path Johnson.
FastPathCircuit.dimensions stores canonical dimension names as
Vec<String> rather than Vec<Dimension> so the salsa-tracked result
type's Debug derive doesn't depend on the engine's debug-derive
feature (off by default in libsimlin's simlin-engine dependency).
Stages 5 and 6 of the design plan.
Same fix as commit e534590 (`serve: clear inherited GIT_* env in git probe and tests`) extended to the watcher_git integration test suite. The `must_git` helper there was not previously updated and inherits the parent's GIT_DIR / GIT_WORK_TREE / GIT_INDEX_FILE when running under the project pre-commit hook (`cargo test --workspace`), which makes the inner git operate on the outer repository instead of the freshly-`git init`'d temp dir.
Stage 7 of the variable-level loop enumeration work: documentation updates and measurement capture. - design-plan postscript records var/element SCC, var-level circuit count, legacy element-level circuit count, fast-path count, slow-path count, and slow-path SCC for the cross_element_ltm and arrayed_population_ltm fixtures. - engine CLAUDE.md describes `model_edge_shapes`, `model_loop_circuits_tiered`, `classify_cycle`, `TieredCircuitsResult`, and the dual-gate auto-flip strategy. - LTM design doc rewrites the loop-score section to describe the PureScalar / PureSameElementA2A / CrossElementOrMixed classification and the slow-path subgraph that drives element-level Johnson on the remaining mixed cycles. The two-tier auto-flip strategy (variable-level early gate, slow-path subgraph late gate) is documented in the divergence-from-papers section. - tech-debt #25 marked RESOLVED with pointers to the design plan and the resolving stages of this PR. - new `measurement_postscript_*` integration tests in simulate_ltm.rs pin the post-refactor numbers so future regressions surface as test failures.
Stage 7 follow-up: the exhaustive-mode flow in docs/design/ltm--loops-that-matter.md still described `model_loop_circuits` directly enumerating elementary circuits. After the tiered refactor, `model_loop_circuits_tiered` is the primary entry point used by `model_ltm_variables`; the design doc now describes the fast / slow path partition and how each is materialized into Loops.
The slow-path subgraph in model_loop_circuits_tiered is induced on every variable that appears in any CrossElementOrMixed cycle. That is intentionally broad -- a variable participating in even one cross-element cycle must keep its element nodes in the subgraph -- but when a pure-A2A cycle shares variables with a cross-element cycle (e.g., "a -> grow -> stock -> b -> a" co-existing with "a -> grow -> stock -> b -> c -> a" where c[r] = b[NYC] + ...), the pure-A2A cycle's nodes get pulled into the subgraph too. Johnson then re-finds the per-element reflections of that pure-A2A cycle, which build_element_level_loops re-collapses into an A2A Loop, duplicating the Loop the fast path already emitted. Filter slow-path circuits whose stripped variable-level node sequence matches a fast-path circuit's node sequence (rotation-invariant via canonical_cycle_rotation). This drops only the per-element reflections; genuine cross-element cycles (which traverse extra variables and therefore have a different stripped sequence) survive the filter. Add a permanent regression fixture in tests/simulate_ltm.rs that constructs the reviewer's reproducer (a/grow/stock/b/c with FixedIndex through b[NYC]) and asserts exactly 2 distinct loops are emitted. The unfixed branch produces 3 (r1, r2, u1).
…string The post-#482 LTM pipeline routes through model_loop_circuits_tiered; model_element_loop_circuits is retained only for diagnostic / measurement-postscript tests and external consumers that want the unfiltered element-level circuit list. Mark it #[deprecated] so new LTM callers get an automatic warning if they wire it up by accident, while keeping it callable for the existing measurement helpers in tests/simulate_ltm.rs, the internal unit tests in db_element_graph_tests and db_ltm_unified_tests, and the ltm_full_bench example. Each remaining caller carries an #[allow(deprecated)] with a comment explaining why it intentionally targets the legacy surface. The salsa::tracked macro expansion references the function by name in its generated code, so the inner #[allow(deprecated)] is required on the function definition itself; an outer-only deprecation lint would fire inside the macro expansion and there is no #[allow] we can apply inside the salsa-generated code. Update MAX_LTM_SCC_NODES's docstring to describe the post-#482 two-granularity gate: variable-level SCC (cheap pre-Johnson) and slow-path subgraph SCC (computed inside the tiered enumerator). Pre-#482 the docstring described a single element-graph SCC gate, which no longer exists.
Tighten three loose ends from the PR #496 review of the variable-level loop enumeration refactor: - build_loops_from_tiered's canonical-name-to-datamodel-name mapping silently fell back to the canonical (lower-cased) string when a fast- path A2A cycle's dimension name didn't appear in dm_dims. Replace with unreachable! that lists the known dimensions, since the only way the fall-back fires is an internal dim-cache or synthesizer mismatch -- a bug we want surfaced at the cause site rather than as a parse error on the downstream loop-score equation. - Fix measurement_postscript_cross_element_ltm's docstring: the post- #482 measurement is var_scc=5 / elem_scc=10, not 4/8. The births flow closes the population<->births A2A pair so it sits in the same SCC as the migration variables. - Mark tech-debt #25 as 'RESOLVED (pending #496 merge)' so the entry re-flips to RESOLVED automatically when the PR lands and doesn't read as resolved on main while the PR is still in review.
The previous serve: clear inherited GIT_* env... commit stripped every env var beginning with GIT_, including author/committer identity (GIT_AUTHOR_*/GIT_COMMITTER_*), transport (GIT_SSH_COMMAND), credential helpers (GIT_ASKPASS), and interaction flags (GIT_TERMINAL_PROMPT). The original motivation only requires neutralizing the path-targeting vars (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY, GIT_COMMON_DIR) that override the per-call current_dir(cwd). Restrict the strip to that set and update the explanatory comment so future code that uses run_git to commit, push, or pull still inherits author/transport/credential config from the calling environment. Promote the constant + helper (strip_git_path_env / GIT_PATH_TARGETING_VARS) to a single source of truth in simlin_serve::git and have both integration tests (tests/git_integration.rs, tests/watcher_git.rs) plus the in-crate tests::must_git helper call it, so the rule cannot drift between sites.
build_loops_from_tiered previously hit unreachable! when a canonical dimension name didn't appear in the datamodel's dimension list, panicking the whole LTM pipeline. The legacy build_element_level_loops path falls back to the canonical name as a recoverable warning -- mirror that behavior so incremental or partially-invalid model states surface as analysis warnings rather than hard crashes.
Add MostlyReinforcing and MostlyBalancing (Rux/Bux) loop polarity
variants and a polarity_confidence f64 (range 0..=1) on
DetectedLoop. from_runtime_scores no longer collapses any sign
change to Undetermined; loops with one dominant polarity above the
configured threshold are classified mostly-R/B with their
confidence ratio.
Threshold POLARITY_CONFIDENCE_THRESHOLD = 0.99 follows
Schoenberg & Eberlein (2020) "Seamlessly Integrating LTM into the
Modeling Process" (section 13.7 of docs/reference/ltm--loops-that-matter.md
and section 7.6 of the thesis summary), which uses the same
|r - |b|| / (r + |b|) ratio to distinguish predominantly-R/B
loops from genuinely mixed ones in simplified CLDs. The papers
describe the cutoff verbally as "above 0.99" while section 13.7's
formula is inclusive; this implementation uses confidence >=
threshold to match the formula and avoid surfacing high-dominance
loops as Undetermined. Filtering on !is_finite() rather than just
is_nan() keeps Inf/-Inf from poisoning the denominator and
producing NaN confidence (which clamp would not repair).
Existing matches on LoopPolarity / DetectedLoopPolarity were made
exhaustive: src/simlin-engine/src/{analysis.rs,db_analysis.rs,
layout/mod.rs,ltm/{graph,types,tests}.rs,ltm_finding.rs},
src/libsimlin/src/analysis.rs (C ABI keeps R/B/U; mostly-R/B
coalesce to their dominant cousins since the FFI struct has no
confidence field today), and the exhaustive test match in
tests/simulate_ltm.rs.
The Python LoopPolarity.from_runtime_scores reimplementation was still using the pre-#485 semantics (collapse any sign change to UNDETERMINED), so Rust and Python silently disagreed on Rux/Bux loops once the engine landed the new variants. Bring the Python port up to the same algorithm: compute the polarity-confidence ratio |r - |b|| / (r + |b|) and classify mostly-* when one polarity dominates above the POLARITY_CONFIDENCE_THRESHOLD = 0.99 cutoff that mirrors the Rust constant. The C FFI struct still has no confidence field, so structural loops delivered through the FFI continue to surface as REINFORCING/BALANCING/ UNDETERMINED (values 0-2). The new MOSTLY_REINFORCING and MOSTLY_BALANCING variants take values 3 and 4 and only arise from the Python-only from_runtime_scores path; downstream consumers like Run._calculate_dominant_periods bucket them with their dominant pure cousin so existing dominance heuristics keep working.
The MCP instructions and the loop-dominance skill markdown still claimed the JSON polarity field is exactly one of three values, but to_loop_summary in simlin-engine emits five strings now (reinforcing, balancing, undetermined, plus mostly_reinforcing / mostly_balancing). Enumerate all five in instructions.md and the skill, and explain when the mostly_* variants are produced (Rux/Bux: dominant polarity with >=0.99 confidence per Schoenberg & Eberlein 2020) so AI agents reading either resource don't treat the new strings as garbage. Note that Model.loops (structural-only) still reports just the legacy three because the MOSTLY_* variants require simulated runtime scores; the skill text spells this out so callers know which surface they're on.
PR #490 implements the Eberlein/Schoenberg polarity-confidence ratio (`|r - |b|| / (r + |b|)`) and the new MostlyReinforcing/MostlyBalancing variants on DetectedLoop. The tech-debt entry was last reviewed before the implementation landed; refresh the description to point at the in-flight branch and bump the review date so the entry will be marked RESOLVED with the merge SHA once the PR lands.
50c07a4 to
427dbaa
Compare
bpowers
left a comment
bpowers
left a comment
There was a problem hiding this comment.
Rebased onto PR #496 (stacked PR)
This PR is now stacked on top of #496 (ltm/482-variable-level-loops-first). When #496 merges to main, GitHub will auto-update this PR's base back to main.
- Carried the LoopPolarity / from_runtime_scores / POLARITY_CONFIDENCE_THRESHOLD work onto the new ltm/types.rs location (post-#498 module split).
- Dropped the older redundant vars()-based GIT_* commit; the narrowed-allowlist version on #496 (and main, eventually) supersedes it.
- All other changes (db_analysis.rs polarity_confidence field, pysimlin port, MCP loop-dominance skill, docs/tech-debt) preserved verbatim in their unchanged locations.
mergeable: MERGEABLE on top of base ltm/482-variable-level-loops-first.
[P2] Python
|
1 participant
Summary
LoopPolarity::from_runtime_scorespreviously collapsed any sign change in aloop's score series to
Undetermined, dropping useful information about loopsthat briefly express the opposite polarity. This change retains the
Eberlein/Schoenberg polarity-confidence ratio
|r - |b|| / (r + |b|)alongsidethe categorical classification and adds two new variants --
MostlyReinforcing("Rux") and
MostlyBalancing("Bux") -- for loops where one polarity dominatesabove a documented threshold.
DetectedLoopgains apolarity_confidence: f64field in[0.0, 1.0]. Thestructural pipeline currently emits 1.0 for fully-determined R/B loops and 0.0
for U loops; runtime-aware consumers can layer the
from_runtime_scoresclassification on top to surface mostly-R/B loops with their actual confidence
ratio. Existing R/B/U variants are unchanged, so persisted loop names and
deterministic IDs (r1, b1, ...) stay stable.
Threshold
POLARITY_CONFIDENCE_THRESHOLD = 0.99. The value followsEberlein & Schoenberg (2020) "Seamlessly Integrating LTM into the Modeling
Process", which uses the same ratio and the same 0.99 cutoff to distinguish
predominantly-R/B loops (Rux/Bux) from genuinely mixed (Ux) loops in
simplified CLDs. References:
docs/reference/ltm--loops-that-matter.mdsection 13.7 (formula)docs/reference/papers/schoenberg2020.2-thesis--summary.mdsection 7.6(Rx / Bx / Rux / Bux / Ux labelling)
Files updated to handle the new variants
This is an additive enum change (existing R/B/U variants are unchanged), but
existing matches on
LoopPolarity/DetectedLoopPolarityare no longerexhaustive. The following sites now handle the new variants explicitly:
src/simlin-engine/src/ltm.rs--assign_loop_ids(R/B counters sharewith mostly-R/B),
from_runtime_scoresreturnsOption<(LoopPolarity, f64)>src/simlin-engine/src/ltm_finding.rs-- updatedassign_loop_idsand thecall site that previously consumed the old
Option<LoopPolarity>returnsrc/simlin-engine/src/db_analysis.rs-- structural pipeline emitsconfidence 1.0 / 0.0 alongside polarity;
DetectedLoopandDetectedLoopPolarityextended;Eqderive dropped onDetectedLoopandDetectedLoopsResultbecause of thef64fieldsrc/simlin-engine/src/analysis.rs--to_feedback_loopcoalesces mostly-R/Bto R/B for the layout-metadata enum;
to_loop_summarypreserves thedistinction in JSON output as
mostly_reinforcing/mostly_balancingsrc/simlin-engine/src/layout/mod.rs-- mostly-R/B coalesce to R/B for themetadata::LoopPolarityenum used by the layout pipelinesrc/libsimlin/src/analysis.rs-- C ABI keeps R/B/U; mostly-R/B coalesceto their dominant cousins to avoid a breaking change for CGo / WASM / C
consumers (the FFI struct has no confidence field today)
src/simlin-engine/tests/simulate_ltm.rs-- exhaustive diagnostic match inensure_ltm_resultsextendedTests
LoopPolarity::from_runtime_scoresunit tests inltm.rs:reinforcing/balancing/mostly-R/mostly-B/symmetric-undetermined/
weakly-dominant-undetermined/empty/all-NaN/all-zero
polarity_confidence_testsmodule indb_analysis.rs:structural_reinforcing_loop_has_full_confidence,structural_balancing_loop_has_full_confidence,structural_undetermined_loop_has_zero_confidence(uses non-monotone GF),runtime_mostly_reinforcing_confidence_above_threshold,small_simulated_model_surfaces_consistent_confidence(compiles and runsa tiny logistic-style model; verifies structural and runtime confidence
agree at 1.0 for each unambiguous loop)
Test plan
cargo test -p simlin-engine(lib + integration) passescargo test -p simlin-engine --features file_io(LTM integration tests) passescargo clippy --workspace --all-targets -- -D warningscleancargo fmt --checkcleanFixes #485