Skip to content

Commit f0e6dc3

Browse files
committed
feat(delphi): D10 — Clojure-parity rep comment selection (PR 8)
Replaces the pre-D10 botched-port `select_rep_comments_df` with a single-pass reduce that mirrors Clojure `select-rep-comments` (math/src/polismath/math/repness.clj:212-281). Sits on top of PR 14a in the spr stack. ## Helpers added (top-level in `repness.py`) - `passes_by_test(s)` — Clojure `passes-by-test?` (repness.clj:165-170). OR'd on (rat, pat) and (rdt, pdt) z-sig-90. NO `pa >= 0.5` gate; the pre-D10 Python gate was an over-restriction with no Clojure analog. - `beats_best_by_test(s, current_best_z)` — Clojure `beats-best-by-test?` (repness.clj:133-139). Strict `>` on `max(rat, rdt)`. - `beats_best_agr(s, current_best)` — Clojure `beats-best-agr?` (repness.clj:142-162). Four-branch agree-priority logic: 1. na == 0 AND nd == 0 → reject. 2. current_best AND current_best.ra > 1.0 → compare 4-way signed product `ra * rat * pa * pat`. 3. current_best (else, ra <= 1.0) → compare `pa * pat` only. 4. No current_best → accept if `z90(pat)` OR `(ra > 1.0 AND pa > 0.5)`. `current_best` stores the RAW row (Clojure repness.clj:250) so the ra/rat/pa/pat surface stays available across iterations. - `_finalize_row_for_output(row, *, is_best_agree=False)` — Clojure `finalize-cmt-stats` (repness.clj:173-188) + best-agree flagging (repness.clj:262-264). Emits `best_agree=True` and `n_agree=na` for the best-agree slot. ## `select_rep_comments_df` rewrite Signature now: `(stats_df, mod_out=None) -> List[Dict[str, Any]]`. Drops the `agree_count` / `disagree_count` kwargs (Clojure has only a cap of 5). Per-row state `{sufficient, best, best_agree}` updated by the helpers. Final assembly: dedup best_agree from sufficient → sort by metric (agree_metric for repful=='agree', disagree_metric for 'disagree') → prepend finalized+flagged best_agree → take 5 → agrees-before-disagrees. The caller in `conv_repness` drops the `_stats_row_to_dict` wrapping step (the new function returns finalized dicts directly). ## Two pre-D10 bugs fixed alongside the rewrite - `pa >= 0.5 / pd >= 0.5` over-gate in the passing filter — removed (no Clojure analog). - "Fill from other category" + "first row" fallback blocks — deleted. The `:best` / `:best_agree` mechanism IS the Clojure fallback. ## Tests (18 new in `tests/test_discrepancy_fixes.py`) - TestD10PassesByTest (4): agree-side, disagree-side, neither, no pa-gate. - TestD10BeatsBestByTest (3): None-best, max(rat,rdt), strict `>`. - TestD10BeatsBestAgr (6): one per Clojure branch + boundary. - TestD10SelectRepCommentsBoundary (5): empty input, single unvoted row → best fallback, sufficient-empty-best-agree-only, take-5 cap + agrees-before-disagrees, **the eviction edge case** (best_agree outside sufficient evicting 5th-highest-metric). ## Eviction edge case — flagged `take(5)` runs AFTER prepending best_agree. If `best_agree` was kept by `beats_best_agr` as a non-significant agree-priority fallback (failed `passes_by_test`, qualified via Branch 4) AND `:sufficient` already has 5 entries, the prepend pushes total to 6 and `take(5)` evicts the 5th-highest-metric sufficient entry — possibly a strong dissenting view. Mirrors Clojure exactly for blob parity. `# TODO(parity-eviction)` comment at the take(5) site in the production code; entry added under "Pending — needs team discussion" in PLAN.md; pinned by the synthetic test above. ## Re-xfailed with updated reasons (D14 / D1 upstream divergence) Six per-shared-(gid, tid) blob-comparison tests were previously xfailed as "D5/D6/D7/D8/D10: no shared comments to compare". After D10 there ARE shared comments (overlap ~20% on vw cold_start), but the per-(gid, tid) stats still mismatch because Python and Clojure place different participants in the "same" group ID. That's upstream PCA/KMeans group-membership divergence (D14 / D1), not D10. Updated xfail reasons point at D14 / D1. D10 itself is verified by the 18 synthetic tests. Affected: TestD9ZScoreThresholds::test_z_values_match_clojure, TestD5ProportionTest::test_pat_values_match_clojure_blob, TestD6TwoPropTest::test_rat_values_match_clojure_blob, TestD7RepnessMetric::test_repness_metric_matches_clojure_blob, TestD8FinalizeStats::test_repful_matches_clojure_blob, TestD10RepCommentSelection::test_rep_comments_match_clojure. ## Suite delta - Pre (post-14a baseline): 295 passed, 12 skipped, 58 xfailed. - Post: 313 passed, 12 skipped, 58 xfailed. - Delta: +18 passed (the 18 new D10 synthetic tests), 0 failed, no new xfailed. ## Documentation - `delphi/docs/PLAN_DISCREPANCY_FIXES.md`: PR 14a row marked landed (#2564), PR 8 (D10) marked in-flight, eviction concern added to "Pending — needs team discussion". - `delphi/docs/CLJ-PARITY-FIXES-JOURNAL.md`: "Session: PR 8 — D10 rep comment selection (2026-06-11)" entry with full scope, suite delta, and decisions log pointer. ## /goal mode This PR is part of an autonomous run (`/goal`) targeting D10 + D11 + D12 + golden snapshots as a stacked PR series. Decisions made autonomously (key naming, return type, etc.) are documented in `~/polis/D10_D11_D12_GOLDENS_DECISIONS.md` for batch user review at the end of the run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> commit-id:1b681fef
1 parent b3c3c4f commit f0e6dc3

4 files changed

Lines changed: 628 additions & 107 deletions

File tree

delphi/docs/CLJ-PARITY-FIXES-JOURNAL.md

Lines changed: 100 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1406,5 +1406,103 @@ sklearn-KMeans-seeding consensus (see scratch/COPILOT_MATH_QUESTIONS.md);
14061406
no values shift at the goldens commit until D10/D11/D12 land.
14071407

14081408
**Stack position.** New commit inserted between PR 14a (#2564) and D10
1409-
(#2566). D10, D11, D12, goldens rebased cleanly on top — no conflict
1410-
markers in `jj log -r 'edge..@+++++'`.
1409+
(#2566). D10, D11, D12, goldens rebased cleanly on top; 2-sided docs
1410+
conflicts in PLAN/JOURNAL at each downstream commit resolved manually
1411+
to merge both edits (downstream docs additions kept; ns-PASS row and
1412+
session entry preserved).
1413+
1414+
## Session: PR 8 — D10 rep comment selection (2026-06-11)
1415+
1416+
Landed in `/goal` mode (autonomous run targeting D10 + D11 + D12 + goldens
1417+
as a stacked PR series). Decisions made without inline user check are
1418+
documented in `~/polis/D10_D11_D12_GOLDENS_DECISIONS.md` for batch review.
1419+
1420+
### What landed
1421+
1422+
**Production code (`delphi/polismath/pca_kmeans_rep/repness.py`)** — added
1423+
3 top-level helpers + `_finalize_row_for_output` + rewrote `select_rep_comments_df`:
1424+
1425+
- `passes_by_test(s) -> bool` — Clojure `passes-by-test?` (repness.clj:165-170).
1426+
OR'd on `(rat, pat)` and `(rdt, pdt)` z-sig-90. **NO `pa >= 0.5` gate**
1427+
the pre-D10 Python gate was a botched-port over-restriction with no
1428+
Clojure analog.
1429+
- `beats_best_by_test(s, current_best_z) -> bool` — Clojure `beats-best-by-test?`
1430+
(repness.clj:133-139). Strict `>` on `max(rat, rdt)` vs current best z.
1431+
- `beats_best_agr(s, current_best) -> bool` — Clojure `beats-best-agr?`
1432+
(repness.clj:142-162). Four-branch agree-priority logic:
1433+
1. `na == 0 and nd == 0` → reject.
1434+
2. Current best AND `current_best['ra'] > 1.0` → compare 4-way signed
1435+
product `ra * rat * pa * pat`.
1436+
3. Current best (else, `ra <= 1.0`) → compare `pa * pat`.
1437+
4. No current best → accept if `z90(pat)` OR `(ra > 1.0 AND pa > 0.5)`.
1438+
- `_finalize_row_for_output(row, *, is_best_agree=False)` — Clojure
1439+
`finalize-cmt-stats` (repness.clj:173-188) + best-agree flagging
1440+
(repness.clj:262-264). Adds `best_agree=True` and `n_agree=na` keys for
1441+
the best-agree slot.
1442+
- `select_rep_comments_df(stats_df, mod_out=None) -> List[Dict[str, Any]]`
1443+
single-pass reduce over `stats_df.to_dict('records')` mirroring Clojure
1444+
`select-rep-comments` (repness.clj:212-281). Per-row state
1445+
`{sufficient, best, best_agree}` updated by the three helpers; final
1446+
assembly is dedup-best-agree-from-sufficient → sort by metric →
1447+
prepend best-agree → take 5 → agrees-before-disagrees.
1448+
1449+
**Caller (`conv_repness`)** — dropped the `_stats_row_to_dict` wrapping
1450+
step since `select_rep_comments_df` now returns finalized dicts directly.
1451+
1452+
**Two pre-D10 bugs fixed alongside the rewrite** (research-agent flagged):
1453+
- `pa >= 0.5 / pd >= 0.5` over-gate in the passing filter — removed. No
1454+
Clojure analog; was dropping legitimate candidates.
1455+
- "Fill-from-other-category" + "first-row" fallback blocks — deleted. The
1456+
`:best` / `:best_agree` mechanism IS the Clojure fallback.
1457+
1458+
**Tests** — 18 new tests (in `tests/test_discrepancy_fixes.py`):
1459+
- `TestD10PassesByTest` (4 tests): agree-side significant, disagree-side
1460+
significant, neither significant, no `pa >= 0.5` gate.
1461+
- `TestD10BeatsBestByTest` (3 tests): None-best, max-rat-rdt, strict `>`.
1462+
- `TestD10BeatsBestAgr` (6 tests): Branch 1 (na=nd=0), Branch 2 (ra>1),
1463+
Branch 3 (ra<=1), Branch 4 z90(pat), Branch 4 (ra>1 AND pa>0.5), Branch 4
1464+
rejection.
1465+
- `TestD10SelectRepCommentsBoundary` (5 tests): empty input, single unvoted
1466+
row → best fallback, sufficient-empty-best-agree-only, take-5 cap with
1467+
agrees-before-disagrees ordering, **the eviction edge case** (best_agree
1468+
outside sufficient evicting 5th-highest-metric).
1469+
1470+
**Re-xfailed with updated reasons** (D14 / D1 upstream divergence):
1471+
- `TestD9ZScoreThresholds::test_z_values_match_clojure`
1472+
- `TestD5ProportionTest::test_pat_values_match_clojure_blob`
1473+
- `TestD6TwoPropTest::test_rat_values_match_clojure_blob`
1474+
- `TestD7RepnessMetric::test_repness_metric_matches_clojure_blob`
1475+
- `TestD8FinalizeStats::test_repful_matches_clojure_blob`
1476+
- `TestD10RepCommentSelection::test_rep_comments_match_clojure`
1477+
1478+
Why xfailed despite D10 landing: D10 enables shared comments in the
1479+
selection (overlap rises from 0% to ~20% on vw cold_start), but
1480+
per-(gid, tid) stats still mismatch because Python and Clojure put
1481+
different participants in the "same" group ID. That's upstream
1482+
PCA/KMeans group-membership divergence (D14 / D1), not D10. D10 is
1483+
verified via the 18 synthetic helper + boundary tests above.
1484+
1485+
### Suite delta (pre/post D10)
1486+
1487+
- Pre (post-14a): 295 passed, 12 skipped, 58 xfailed.
1488+
- Post (this PR): 313 passed, 12 skipped, 58 xfailed.
1489+
- Delta: +18 passed, 0 failed, 0 new xfailed. The +18 matches the 18 new
1490+
D10 synthetic tests exactly.
1491+
1492+
### Decisions made autonomously (under `/goal` mode)
1493+
1494+
See `~/polis/D10_D11_D12_GOLDENS_DECISIONS.md`. Highlights:
1495+
- **S1**: Python convention key names (`repful`, `best_agree`, `n_agree`)
1496+
instead of Clojure hyphens. Math blob alignment is a future PR.
1497+
- **S2**: `select_rep_comments_df` returns `List[Dict[str, Any]]` instead
1498+
of `pd.DataFrame` — variable extra keys (best_agree flag) make list-of-
1499+
dicts cleaner than DF-with-NaN-columns.
1500+
- **D10.1**: Two pre-D10 bugs (pa>=0.5 gate, fill-from-other fallback)
1501+
folded into D10 rather than separate PRs — the rewrite replaces the
1502+
function so a surgical fix would be more noise than value.
1503+
- **D10.7**: Real-data blob-comparison tests re-xfailed with reasons
1504+
pointing at D14/D1, not softened to overlap-thresholds — more honest.
1505+
1506+
### What's Next
1507+
1508+
PR 9 (D11) on top of D10 in the same spr stack.

delphi/docs/PLAN_DISCREPANCY_FIXES.md

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,9 @@ This plan's "PR N" labels map to actual GitHub PRs as follows:
2828
| PR 7 (D8) | #2522 | Stack 15/17 | Fix D8: finalize comment stats |
2929
| PR 12 (D15) | #2523 | Stack 16/17 | Fix D15: moderation handling |
3030
| (K-inv) | #2524 | Stack 17/17 | Fix K-means k divergence: preserve row order |
31-
| PR 14a (scalar deletion) | — (in flight) || Delete dead scalar paths in `repness.py`; migrate blob injection tests to vectorized |
32-
| PR ns-PASS fix | — (planned) || Fix `ns` to include PASS votes in `compute_group_comment_stats_df` (Clojure `count-votes` parity) |
33-
| PR 8 (D10) | — (WIP) || Fix D10: rep comment selection — **NEEDS REWORK** |
31+
| PR 14a (scalar deletion) | #2564 || Delete dead scalar paths in `repness.py`; migrate blob injection tests to vectorized |
32+
| PR ns-PASS fix | — (in flight) || Fix `ns` to include PASS votes in `compute_group_comment_stats_df` (Clojure `count-votes` parity) |
33+
| PR 8 (D10) | — (in flight) || Fix D10: rep comment selection — single-pass reduce matching Clojure |
3434
| PR 9 (D11) | — (WIP) || Fix D11: consensus selection — **NEEDS REWORK** |
3535
| PR 10 (D3) | — (WIP) || Fix D3: k-smoother buffer — **NEEDS REWORK** |
3636
| PR 11 (D12) | — (WIP) || Fix D12: comment priorities — **NEEDS REWORK** |
@@ -916,3 +916,14 @@ Tagging this as a follow-up. No code changes until we discuss.
916916
- **`to_dynamo_dict` parallel inline implementations** were refactored to
917917
route through the same helpers as `to_dict` in PR #2523 follow-up. No
918918
further action needed.
919+
- **D10 take-5 eviction edge case** (2026-06-11). The Clojure-parity
920+
`select_rep_comments_df` introduced in PR 8 mirrors Clojure exactly:
921+
`take(5)` runs AFTER prepending the `best_agree` slot. When `best_agree`
922+
was kept by `beats_best_agr?` as a non-significant agree-priority fallback
923+
(i.e. it failed `passes_by_test?` but qualified via Branch 4) AND
924+
`:sufficient` already has 5 entries, the prepend pushes the total to 6
925+
and `take(5)` silently evicts the 5th-highest-metric `sufficient` entry
926+
— possibly a strong dissenting view. Mirrored for blob parity; flag for
927+
future product review. See `# TODO(parity-eviction)` in
928+
`delphi/polismath/pca_kmeans_rep/repness.py::select_rep_comments_df` and
929+
the synthetic test `TestD10SelectRepCommentsBoundary::test_take_5_eviction_when_best_agree_outside_sufficient`.

0 commit comments

Comments
 (0)