Skip to content

Commit 47c6c6b

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 05dae1d commit 47c6c6b

4 files changed

Lines changed: 623 additions & 104 deletions

File tree

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

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1347,3 +1347,99 @@ PR 14a unblocks (in stack order):
13471347
readability reference. Research agent produced a clean split proposal:
13481348
`_build_group_comment_index` (plumbing) + `_compute_per_group_stats`
13491349
(math) + 5-line orchestrator.
1350+
1351+
## Session: PR 8 — D10 rep comment selection (2026-06-11)
1352+
1353+
Landed in `/goal` mode (autonomous run targeting D10 + D11 + D12 + goldens
1354+
as a stacked PR series). Decisions made without inline user check are
1355+
documented in `~/polis/D10_D11_D12_GOLDENS_DECISIONS.md` for batch review.
1356+
1357+
### What landed
1358+
1359+
**Production code (`delphi/polismath/pca_kmeans_rep/repness.py`)** — added
1360+
3 top-level helpers + `_finalize_row_for_output` + rewrote `select_rep_comments_df`:
1361+
1362+
- `passes_by_test(s) -> bool` — Clojure `passes-by-test?` (repness.clj:165-170).
1363+
OR'd on `(rat, pat)` and `(rdt, pdt)` z-sig-90. **NO `pa >= 0.5` gate**
1364+
the pre-D10 Python gate was a botched-port over-restriction with no
1365+
Clojure analog.
1366+
- `beats_best_by_test(s, current_best_z) -> bool` — Clojure `beats-best-by-test?`
1367+
(repness.clj:133-139). Strict `>` on `max(rat, rdt)` vs current best z.
1368+
- `beats_best_agr(s, current_best) -> bool` — Clojure `beats-best-agr?`
1369+
(repness.clj:142-162). Four-branch agree-priority logic:
1370+
1. `na == 0 and nd == 0` → reject.
1371+
2. Current best AND `current_best['ra'] > 1.0` → compare 4-way signed
1372+
product `ra * rat * pa * pat`.
1373+
3. Current best (else, `ra <= 1.0`) → compare `pa * pat`.
1374+
4. No current best → accept if `z90(pat)` OR `(ra > 1.0 AND pa > 0.5)`.
1375+
- `_finalize_row_for_output(row, *, is_best_agree=False)` — Clojure
1376+
`finalize-cmt-stats` (repness.clj:173-188) + best-agree flagging
1377+
(repness.clj:262-264). Adds `best_agree=True` and `n_agree=na` keys for
1378+
the best-agree slot.
1379+
- `select_rep_comments_df(stats_df, mod_out=None) -> List[Dict[str, Any]]`
1380+
single-pass reduce over `stats_df.to_dict('records')` mirroring Clojure
1381+
`select-rep-comments` (repness.clj:212-281). Per-row state
1382+
`{sufficient, best, best_agree}` updated by the three helpers; final
1383+
assembly is dedup-best-agree-from-sufficient → sort by metric →
1384+
prepend best-agree → take 5 → agrees-before-disagrees.
1385+
1386+
**Caller (`conv_repness`)** — dropped the `_stats_row_to_dict` wrapping
1387+
step since `select_rep_comments_df` now returns finalized dicts directly.
1388+
1389+
**Two pre-D10 bugs fixed alongside the rewrite** (research-agent flagged):
1390+
- `pa >= 0.5 / pd >= 0.5` over-gate in the passing filter — removed. No
1391+
Clojure analog; was dropping legitimate candidates.
1392+
- "Fill-from-other-category" + "first-row" fallback blocks — deleted. The
1393+
`:best` / `:best_agree` mechanism IS the Clojure fallback.
1394+
1395+
**Tests** — 18 new tests (in `tests/test_discrepancy_fixes.py`):
1396+
- `TestD10PassesByTest` (4 tests): agree-side significant, disagree-side
1397+
significant, neither significant, no `pa >= 0.5` gate.
1398+
- `TestD10BeatsBestByTest` (3 tests): None-best, max-rat-rdt, strict `>`.
1399+
- `TestD10BeatsBestAgr` (6 tests): Branch 1 (na=nd=0), Branch 2 (ra>1),
1400+
Branch 3 (ra<=1), Branch 4 z90(pat), Branch 4 (ra>1 AND pa>0.5), Branch 4
1401+
rejection.
1402+
- `TestD10SelectRepCommentsBoundary` (5 tests): empty input, single unvoted
1403+
row → best fallback, sufficient-empty-best-agree-only, take-5 cap with
1404+
agrees-before-disagrees ordering, **the eviction edge case** (best_agree
1405+
outside sufficient evicting 5th-highest-metric).
1406+
1407+
**Re-xfailed with updated reasons** (D14 / D1 upstream divergence):
1408+
- `TestD9ZScoreThresholds::test_z_values_match_clojure`
1409+
- `TestD5ProportionTest::test_pat_values_match_clojure_blob`
1410+
- `TestD6TwoPropTest::test_rat_values_match_clojure_blob`
1411+
- `TestD7RepnessMetric::test_repness_metric_matches_clojure_blob`
1412+
- `TestD8FinalizeStats::test_repful_matches_clojure_blob`
1413+
- `TestD10RepCommentSelection::test_rep_comments_match_clojure`
1414+
1415+
Why xfailed despite D10 landing: D10 enables shared comments in the
1416+
selection (overlap rises from 0% to ~20% on vw cold_start), but
1417+
per-(gid, tid) stats still mismatch because Python and Clojure put
1418+
different participants in the "same" group ID. That's upstream
1419+
PCA/KMeans group-membership divergence (D14 / D1), not D10. D10 is
1420+
verified via the 18 synthetic helper + boundary tests above.
1421+
1422+
### Suite delta (pre/post D10)
1423+
1424+
- Pre (post-14a): 295 passed, 12 skipped, 58 xfailed.
1425+
- Post (this PR): 313 passed, 12 skipped, 58 xfailed.
1426+
- Delta: +18 passed, 0 failed, 0 new xfailed. The +18 matches the 18 new
1427+
D10 synthetic tests exactly.
1428+
1429+
### Decisions made autonomously (under `/goal` mode)
1430+
1431+
See `~/polis/D10_D11_D12_GOLDENS_DECISIONS.md`. Highlights:
1432+
- **S1**: Python convention key names (`repful`, `best_agree`, `n_agree`)
1433+
instead of Clojure hyphens. Math blob alignment is a future PR.
1434+
- **S2**: `select_rep_comments_df` returns `List[Dict[str, Any]]` instead
1435+
of `pd.DataFrame` — variable extra keys (best_agree flag) make list-of-
1436+
dicts cleaner than DF-with-NaN-columns.
1437+
- **D10.1**: Two pre-D10 bugs (pa>=0.5 gate, fill-from-other fallback)
1438+
folded into D10 rather than separate PRs — the rewrite replaces the
1439+
function so a surgical fix would be more noise than value.
1440+
- **D10.7**: Real-data blob-comparison tests re-xfailed with reasons
1441+
pointing at D14/D1, not softened to overlap-thresholds — more honest.
1442+
1443+
### What's Next
1444+
1445+
PR 9 (D11) on top of D10 in the same spr stack.

delphi/docs/PLAN_DISCREPANCY_FIXES.md

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ 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 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 8 (D10) | — (in flight) || Fix D10: rep comment selection — single-pass reduce matching Clojure |
3333
| PR 9 (D11) | — (WIP) || Fix D11: consensus selection — **NEEDS REWORK** |
3434
| PR 10 (D3) | — (WIP) || Fix D3: k-smoother buffer — **NEEDS REWORK** |
3535
| PR 11 (D12) | — (WIP) || Fix D12: comment priorities — **NEEDS REWORK** |
@@ -915,3 +915,14 @@ Tagging this as a follow-up. No code changes until we discuss.
915915
- **`to_dynamo_dict` parallel inline implementations** were refactored to
916916
route through the same helpers as `to_dict` in PR #2523 follow-up. No
917917
further action needed.
918+
- **D10 take-5 eviction edge case** (2026-06-11). The Clojure-parity
919+
`select_rep_comments_df` introduced in PR 8 mirrors Clojure exactly:
920+
`take(5)` runs AFTER prepending the `best_agree` slot. When `best_agree`
921+
was kept by `beats_best_agr?` as a non-significant agree-priority fallback
922+
(i.e. it failed `passes_by_test?` but qualified via Branch 4) AND
923+
`:sufficient` already has 5 entries, the prepend pushes the total to 6
924+
and `take(5)` silently evicts the 5th-highest-metric `sufficient` entry
925+
— possibly a strong dissenting view. Mirrored for blob parity; flag for
926+
future product review. See `# TODO(parity-eviction)` in
927+
`delphi/polismath/pca_kmeans_rep/repness.py::select_rep_comments_df` and
928+
the synthetic test `TestD10SelectRepCommentsBoundary::test_take_5_eviction_when_best_agree_outside_sufficient`.

0 commit comments

Comments
 (0)