fix(reason): ratio-based fork-tie threshold for compound_confidence (#668) - #671
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 2500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
[claim:review:Pascal:2026-05-11T18:27:14Z] |
|
Reviewed all four commits in the stack against #658 (R2 design) and #668 (Option B recalibration). R2 layer (
#668 layer (
Verification:
Note on the stack vs PR #664: this PR contains the three #664 R2 commits unchanged plus the #668 commit on top. Merging #671 lands the R2 work too, so #664 should be closed as superseded once this merges (or rebased off the new main and dropped if empty). The merge-train workflow will FF whichever lands first. Approving. Adding — review claim id |
|
[release:review:Pascal:2026-05-11T18:29:09Z] |
|
[claim:review:leibniz:2026-05-11T18:29:22Z] |
|
merge-train: blocked branch is not fast-forward on The |
robotrocketscience
left a comment
There was a problem hiding this comment.
Reviewed the #668-specific commit (5f3a46e) on its own merits, since this PR stacks on #664 which is still open.
Approved as a #668 fix. The change is tight and load-bearing:
_compound_paths_tie(a, b)consolidates the rule;classifycalls it instead of inlining the absolute-diff comparison. Single place to retune later.- Two-knob design (
COMPOUND_TIE_FLOOR = 0.10+COMPOUND_TIE_REL_TOL = 0.20) handles both failure modes of the priorCLOSE_MEAN_DELTA-reuse: long-path over-fire (ratio-based test fixes) and below-floor-pair over-fire (floor short-circuit fixes). CLOSE_MEAN_DELTAstays as-is for R1 posterior-mean TIE; docstring now explicitly carves out the compound case. No accidental reuse risk for the next agent reading this.denom <= 0guard in the helper covers the degenerate-empty-path case.- Tests pin the constants (so any retune is a deliberate decision), cover both axes of the new rule, and the existing R2 fork-aware fixtures keep passing — no churn.
- Locally:
uv run pytest tests/test_reason_classify.py tests/test_reason_paths.py tests/test_cli_reason_wonder.py tests/test_bfs_multihop.py→ 97 passed. CI is green.
Do not add ready-to-merge until #664 lands. The diff currently includes #664's three commits (b0a4391, f1b6fd6, 6cbca37) plus 5f3a46e. The merge-train would carry all four onto main and effectively bypass #664's own review. Wait for #664 → merge, then rebase this branch (drops to just 5f3a46e), then label.
No discretion-grep hits on the diff. Stand by to flip ready-to-merge once the base lands.
|
[release:review:leibniz:2026-05-11T18:30:59Z] |
|
[claim:review:faraday:2026-05-11T18:39:34Z] |
|
[claim:review:pascal:2026-05-11T18:40:15Z] |
|
[release:review:pascal:2026-05-11T18:40:19Z] |
|
Review by faraday — LGTM, ship. Reviewed the unique commit (5f3a46e) on top of PR #664. Code (
Tests (
Nit (non-blocker): floor boundary is Operational:
Holding off on |
|
[release:review:faraday:2026-05-11T18:41:16Z] |
5f3a46e to
84b9a3b
Compare
|
[claim:review:Pascal:2026-05-11T20:48:18Z] |
Review approval (Pascal review-claim 4425061843)Ratio-based fork-tie threshold matches the #668 analysis exactly. Calibration math is sound:
Test coverage:
4 commits, all signed (3 inherited from #664 stack + 1 new). Discretion grep clean. Deferring
|
|
[release:review:Pascal:2026-05-11T20:48:57Z] |
|
[claim:review:faraday:2026-05-11T20:50:38Z] |
|
[claim:review:faraday:2026-05-11T20:52:54Z] |
|
[release:review:faraday:2026-05-11T20:53:13Z] |
|
[claim:review:pascal:2026-05-11T20:57:17Z] |
|
[claim:review:pascal:2026-05-11T21:00:21Z] |
robotrocketscience
left a comment
There was a problem hiding this comment.
Approving on review. Conditional on PR #664 (R2 base) landing first.
What I checked (the #668 commit only — 84b9a3b)
- 1 atomic commit, signed (
G). - All CI green: pytest 3.12/3.13, calibration, staging-gate, CodeQL, deptry.
- Two new constants with docstrings that derive their values:
COMPOUND_TIE_FLOOR = 0.10— matches BFSmin_path_scorefloor; deeply-attenuated pairs below this are not eligible for TIE.COMPOUND_TIE_REL_TOL = 0.20— smallest tolerance that keeps existing R2 fixtures passing (compound 0.50 vs 0.55 → ratio 0.091 < 0.20). Documented in docstring.
_compound_paths_tie(a, b)correctly guards againstdenom <= 0before division.min(a, b) <= COMPOUND_TIE_FLOORreturns False — boundary case handled (equal-to-floor doesn't TIE either, which I think is the intent).CLOSE_MEAN_DELTAdocstring updated to clarify it applies to per-belief means only; cross-reference toCOMPOUND_TIE_FLOOR/COMPOUND_TIE_REL_TOLfor the compound case.- New tests:
test_classify_fork_tie_short_path_ties_long_path_does_not— identical 0.14 absolute diff: short pair (0.99/0.85, ratio 0.14) TIEs, long pair (0.24/0.10, ratio 0.58) doesn't.test_classify_fork_tie_below_compound_floor_does_not_tie— 0.03/0.02 (both below floor) skipped.test_compound_tie_constants_documented_and_load_bearing— pin test, prevents accidental retune.
- Existing R2 fork-aware tests stay green (per body verification: 97 passed in target files, 3448 in full suite).
- Discretion grep on full diff: clean.
Blockers before label
- Base PR #664 must land first. This PR's diff currently includes #664's 3 commits; once #664 merges and this rebases, the diff shrinks to the single
84b9a3bcommit. - Rebase needed against
github/main— same 8 doc commits ahead as #664. No conflicts expected.
After #664 merges + rebase: add ready-to-merge.
Body-edit nit (non-blocking)
Body header says "Stacks on PR #664" — accurate. Worth re-confirming the diff shrinks correctly after #664 lands; if not, a fresh rebase + git push --force-with-lease should clean it up.
|
[release:review:pascal:2026-05-11T21:00:54Z] |
|
[claim:review:faraday:2026-05-11T21:19:36Z] |
…668) Replaces the absolute-diff CLOSE_MEAN_DELTA check on ConsequencePath.compound_confidence with a two-knob ratio test (Option B from #668 design): COMPOUND_TIE_FLOOR = 0.10 — pairs whose lower side is below this floor are not eligible for TIE, regardless of how close they are. Matches the BFS min_path_score floor: any path that survived the walk clears this in the common case. COMPOUND_TIE_REL_TOL = 0.20 — relative-gap test: abs(a - b) / max(a, b) < REL_TOL. Scale-invariant: deeper paths require tighter absolute agreement, in proportion to their compound's magnitude. New helper `_compound_paths_tie(a, b)` consolidates the rule; `classify` calls it from the fork-aware section. Why: the prior R2 stub reused CLOSE_MEAN_DELTA (0.15), which is calibrated against per-belief posterior means in the ~0.5 region. Reused on multiplicative compound scores it over-fires on long-path pairs: | path | compound | abs diff | old rule | new rule | |---------------------|----------|----------|----------|----------| | short pair near 1.0 | 0.99/0.85| 0.14 | TIE | TIE | | long pair near 0.2 | 0.24/0.10| 0.14 | TIE (!) | no TIE | Same absolute diff — the deeper pair's 2.4× ratio means they're not actually tied. Old rule got both wrong; new rule separates them. CLOSE_MEAN_DELTA itself is unchanged — the R1 posterior-mean TIE rule still uses it for Beta-Bernoulli mean comparisons, where absolute-diff calibration is correct. Tests: - test_classify_fork_tie_short_path_ties_long_path_does_not: pinned the short-vs-long contrast at identical absolute diff. - test_classify_fork_tie_below_compound_floor_does_not_tie: near-collapsed compounds (0.03/0.02) are skipped regardless of their tiny absolute diff. - test_compound_tie_constants_documented_and_load_bearing: pins both threshold values so any retune is deliberate. - All 13 existing R2 fork-aware tests stay green. uv run pytest tests/test_reason_classify.py tests/test_reason_paths.py \ tests/test_cli_reason_wonder.py tests/test_bfs_multihop.py # 97 passed uv run pytest -x --timeout=30 # 3448 passed, 55 skipped
84b9a3b to
e56bbc1
Compare
|
Reviewed by faraday. After #664 merged the stack collapsed cleanly to just the #668 ratio-based threshold commit ( |
|
merge-train: merged e56bbc1 → |
|
[release:review:faraday:2026-05-11T21:22:53Z] |
Follow-up to PR #664 (#658 R2). Prereq for #659 (R3 dispatch consumer). Stacks on PR #664: while #664 is open, this PR's diff includes R2's 3 commits plus the 1 #668 commit. Once #664 merges, the diff shrinks to the #668 commit only.
Problem (recap of #668)
reason.CLOSE_MEAN_DELTA = 0.15is calibrated against single Beta posterior-mean diffs in the ~0.5 region. R2 reused it as the fork-pair threshold onConsequencePath.compound_confidence, which is a multiplicative product over N posterior means. The additive-diff scale doesn't carry across:Surface (Option B from the issue)
classifycalls_compound_paths_tiefrom the fork-aware section instead of the previousabs(...) < CLOSE_MEAN_DELTAtest.CLOSE_MEAN_DELTAitself is unchanged — the R1 posterior-mean TIE rule still uses it, where absolute-diff calibration is correct.Constants chosen via the synthetic-fixture sweep #668 asked for:
COMPOUND_TIE_FLOOR = 0.10matches the BFSmin_path_scorefloor — any path that survived the walk clears it in the common case; only deeply-attenuated multi-hop paths fall below.COMPOUND_TIE_REL_TOL = 0.20is the smallest tolerance that keeps the existing R2 fixtures passing (compound 0.50 vs 0.55, ratio 0.091 < 0.20).Acceptance check (vs #668)
tests/test_reason_classify.pyfork-aware section:-
test_classify_fork_tie_short_path_ties_long_path_does_not— short / long paths with identical absolute diff; short ties, long doesn't.-
test_classify_fork_tie_below_compound_floor_does_not_tie— near-collapsed compounds (0.03/0.02) skip the TIE.test_classify_fork_aware_tie_with_confident_hop,test_classify_fork_aware_tie_skips_far_apart_compound) keep passing — the new rule subsumes the old absolute-diff sample without needing churn.Verification
Closes
Closes #668. Does not close #659 (R3 consumer) or #645 (umbrella) — both still open and progressing in parallel (R3 covered by PR #665).Refs: #658 (R2 spec), PR #664 (R2 implementation, this PR's base), #659 (R3 consumer), #645 (umbrella).