Skip to content

Commit 05dae1d

Browse files
committed
refactor(delphi): delete dead scalar paths in repness.py (PR 14a)
The scalar implementations in `delphi/polismath/pca_kmeans_rep/repness.py` were test-only — production calls only `compute_group_comment_stats_df`, `select_rep_comments_df`, and `select_consensus_comments_df` (via `conv_repness`). Maintaining two parallel implementations created "where do I put this helper?" ambiguity for the upcoming D10/D11/D12 fixes (all of which add new helpers to `repness.py`) and obscured the structure of the production path. Foundation pass before D10/D11/D12 + PR 14b/14c. No behavioural change — production path untouched. ## Production code (`delphi/polismath/pca_kmeans_rep/repness.py`) DELETED (445 lines): - Primitives: `prop_test`, `two_prop_test` (no production callers). - Orchestration: `comment_stats`, `add_comparative_stats`, `repness_metric`, `finalize_cmt_stats`, `passes_by_test`, `best_agree`, `best_disagree`, `select_rep_comments`, `select_consensus_comments`. - Unused helper: `calculate_kl_divergence` (no callers anywhere in repo). KEPT: - `z_score_sig_90`, `z_score_sig_95` — trivial threshold checks consumed scalar-side; vectorizing would not save lines. ENRICHED: - `prop_test_vectorized` and `two_prop_test_vectorized` docstrings now embed the scalar-equivalent closed-form algebra. The formulas stay readable even though the scalar functions are gone. ## Tests DELETED entirely: - `tests/test_old_format_repness.py` (557 lines, scalar-only sibling of `test_repness_unit.py`). DELETED classes/methods in `tests/test_repness_unit.py`: - `TestCommentStats`, `TestSelectionFunctions`, `TestConsensusAndGroupRepness` (all scalar-only). - `TestStatisticalFunctions::test_prop_test`, `::test_two_prop_test`. MIGRATED to single vectorized DataFrame calls in `tests/test_discrepancy_fixes.py`: - D4/D5/D6 BlobInjection classes — build a DataFrame from the blob's `repness` entries, run one `prop_test_vectorized` / `two_prop_test_vectorized` call, compare element-wise. Tests the actual production code path, produces cleaner diagnostics via `.to_string()`. - `TestD5ProportionTest::test_prop_test_matches_clojure_formula` (consolidated with the n=0 boundary case). - `TestD6TwoPropTest::test_two_prop_test_matches_clojure_formula` (+ edge cases consolidated, + pi_hat=1 boundary cases). CONSOLIDATED in `tests/test_discrepancy_fixes.py`: - `TestD8FinalizeStats`'s 7 scalar boundary tests collapse into a single parametrized DataFrame test `test_repful_classification_boundary` that exercises the production `np.where(rat > rdt, 'agree', 'disagree')` logic. All boundary cases preserved (rat<rdt, rat>rdt, rat==rdt non-zero, rat==rdt==0, negative z-scores). DELETED redundant tests: - `TestSyntheticEdgeCases::test_prop_test_matches_clojure_formula_synthetic` (duplicated by migrated TestD5ProportionTest). - `TestSyntheticEdgeCases::test_clojure_repness_metric_product` (duplicated by migrated TestD7RepnessMetric). - `TestSyntheticEdgeCases::test_clojure_repful_uses_rat_vs_rdt` (purely tautological). Cross-checks in `tests/test_repness_unit.py::TestVectorizedFunctions` now use `_prop_test_reference` and `_two_prop_test_reference` closed-form staticmethods in place of scalar calls. ## Suite delta - Pre (edge @ 2dce738): 330 passed, 12 skipped, 58 xfailed. - Post: 295 passed, 12 skipped, 58 xfailed. - Delta: -35 passed, 0 failed, 0 new xfailed. Matches deleted scalar-test count exactly. ## For PR 14c (readability refactor — runs later) The deleted scalar code is the readability reference for PR 14c. Retrieve via: git show <this-commit>~1:delphi/polismath/pca_kmeans_rep/repness.py \ | sed -n '161,302p' Specifically (pre-deletion line numbers): `comment_stats` 161-201, `add_comparative_stats` 203-235, `repness_metric` 237-271, `finalize_cmt_stats` 273-301. Clojure originals at `math/src/polismath/math/repness.clj:78-100,173-188,191-200`. ## Documentation - `delphi/docs/PLAN_DISCREPANCY_FIXES.md`: added PR 14a row to the stack cross-reference table. - `delphi/docs/CLJ-PARITY-FIXES-JOURNAL.md`: appended "Session: PR 14a — Scalar deletion (2026-06-11)" entry with the full scope, suite delta, and the `git show` recipe for PR 14c. ## Out of scope (handed off separately) - Pyright pandas-stubs noise (10+ false positives on `pd.DataFrame(columns=...)` and `df['col'] = value` in `compute_group_comment_stats_df`) is pre-existing on edge HEAD; PR #2560's pyright config didn't set rule overrides. Handoff: `~/polis/HANDOFF_PYRIGHT_PANDAS_STUBS.md`. Tracked separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> commit-id:694a6768
1 parent d22c374 commit 05dae1d

6 files changed

Lines changed: 424 additions & 1743 deletions

File tree

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

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,3 +1216,134 @@ ever appears in real Polis data. It does.
12161216
out of this committed doc; the unredacted findings are in Claude's
12171217
per-project memory store (`~/.claude/projects/...`). Open a follow-up
12181218
discussion with the team before any user-facing action.
1219+
1220+
## Session: PR 14a — Scalar deletion (2026-06-11)
1221+
1222+
Foundation pass before D10/D11/D12. The scalar implementations in
1223+
`repness.py` were test-only (production calls only `compute_group_comment_stats_df`
1224+
+ `select_rep_comments_df` + `select_consensus_comments_df` via
1225+
`conv_repness`). Deleting them removes the "where do I put this helper?"
1226+
ambiguity for D10/D11/D12 (which all add new helpers to `repness.py`) and
1227+
shrinks the test surface by ~35 obsolete unit tests.
1228+
1229+
### What landed
1230+
1231+
**Production code (`delphi/polismath/pca_kmeans_rep/repness.py`)** — 445 lines deleted:
1232+
- DELETE primitives: `prop_test`, `two_prop_test` (both also dead in production —
1233+
only test consumers).
1234+
- DELETE orchestration: `comment_stats`, `add_comparative_stats`, `repness_metric`,
1235+
`finalize_cmt_stats`, `passes_by_test`, `best_agree`, `best_disagree`,
1236+
`select_rep_comments`, `select_consensus_comments`.
1237+
- DELETE unused: `calculate_kl_divergence` (no callers anywhere).
1238+
- KEEP: `z_score_sig_90`, `z_score_sig_95` (trivial threshold checks used scalar-side
1239+
in selection logic; vectorizing them would not save lines).
1240+
- ENRICH docstrings: `prop_test_vectorized` and `two_prop_test_vectorized` now
1241+
embed the scalar-equivalent closed-form algebra (so the formula stays readable
1242+
even though the scalar functions are gone). Pattern from the user during the
1243+
PR 14a discussion: "where we cannot achieve readability on the vectorized,
1244+
put in comments showing the non-vectorized equivalent."
1245+
1246+
**Tests** — net -35 passed tests (296 → 295... actually delta is 330 → 295):
1247+
- DELETE entirely: `tests/test_old_format_repness.py` (557 lines, mirror of
1248+
scalar-only tests in `test_repness_unit.py`; the "old format" was the scalar
1249+
dict-in/dict-out API).
1250+
- DELETE classes: `TestCommentStats`, `TestSelectionFunctions`,
1251+
`TestConsensusAndGroupRepness` in `test_repness_unit.py`. Also
1252+
`TestStatisticalFunctions::test_prop_test` and `test_two_prop_test`.
1253+
- MIGRATE D4/D5/D6 BlobInjection (`test_discrepancy_fixes.py:1602+`) from
1254+
per-(gid, tid) scalar loop calls to a single vectorized call on a DataFrame
1255+
built from the blob's `repness` entries. This pattern (1) tests the actual
1256+
production code path, (2) produces a `.to_string()` diagnostic that beats
1257+
hand-formatted f-strings, (3) drops loop overhead.
1258+
- MIGRATE `TestD5ProportionTest::test_prop_test_matches_clojure_formula`,
1259+
`TestD6TwoPropTest::test_two_prop_test_matches_clojure_formula` + edge cases
1260+
to single vectorized calls on N-row DataFrames.
1261+
- CONSOLIDATE `TestD8FinalizeStats`'s 7 scalar boundary tests into one
1262+
parametrized DataFrame test (`test_repful_classification_boundary`) that
1263+
exercises the production `np.where(rat > rdt, 'agree', 'disagree')` logic.
1264+
Boundary cases preserved: `rat < rdt`, `rat > rdt`, `rat == rdt` (non-zero,
1265+
zero), negative z-scores.
1266+
- MIGRATE `TestD7RepnessMetric::test_metric_formula_is_product` to hand-computed
1267+
reference values (`1.3*1.8*0.8*2.5 = 4.68` for agree, `0.7*-0.9*0.2*-1.5 = 0.189`
1268+
for disagree — signed product).
1269+
- DELETE redundant scalar-formula tests in `TestSyntheticEdgeCases`
1270+
(test_prop_test_matches_clojure_formula_synthetic — duplicated by migrated
1271+
TestD5ProportionTest; test_clojure_repness_metric_product — duplicated by
1272+
TestD7RepnessMetric; test_clojure_repful_uses_rat_vs_rdt — purely tautological).
1273+
- Rename misleading `test_compute_group_comment_stats_matches_scalar`
1274+
`test_compute_group_comment_stats_consistency_with_conv_repness`.
1275+
- Cross-checks in `TestVectorizedFunctions` (test_repness_unit.py) replaced
1276+
inline scalar calls with `_prop_test_reference` / `_two_prop_test_reference`
1277+
closed-form staticmethods.
1278+
1279+
### Suite delta (pre/post PR 14a)
1280+
1281+
- Pre-baseline (edge @ 2dce7385f): **330 passed, 12 skipped, 58 xfailed**.
1282+
- Post (@ this PR): **295 passed, 12 skipped, 58 xfailed**.
1283+
- Delta: -35 passed, 0 failed, 0 new xfailed. The -35 matches the deleted
1284+
scalar-only test count (test_old_format_repness ~20 + scalar classes in
1285+
test_repness_unit ~9 + scalar test methods in test_discrepancy_fixes ~6
1286+
consolidated/removed).
1287+
1288+
### For PR 14c (readability refactor)
1289+
1290+
PR 14c will refactor `compute_group_comment_stats_df` for readability and
1291+
needs to mirror the scalar recipe. **The deleted scalar code is the
1292+
reference.** Retrieve via:
1293+
1294+
```bash
1295+
git show <PR-14a-commit>~1:delphi/polismath/pca_kmeans_rep/repness.py \
1296+
| sed -n '161,302p'
1297+
```
1298+
1299+
Specifically (using the pre-deletion line numbers — file was 1008 lines at
1300+
edge HEAD 2dce7385f):
1301+
1302+
- `comment_stats` lines 161-201 — the per-(group, comment) recipe.
1303+
- `add_comparative_stats` lines 203-235 — in-vs-out comparison.
1304+
- `repness_metric` lines 237-271 — the `r*rt*p*pt` product.
1305+
- `finalize_cmt_stats` lines 273-301 — agree-vs-disagree branch.
1306+
1307+
The pre-PR-14a commit hash will be the parent of PR 14a's commit. Clojure
1308+
originals: `math/src/polismath/math/repness.clj:78-100,173-188,191-200`.
1309+
1310+
### Pyright noise (unrelated to PR 14a, raised same session)
1311+
1312+
Discovered during PR 14a that pyright produces ~10 errors on `repness.py` from
1313+
pandas-stubs false positives (`pd.DataFrame(columns=...)`, `df['col'] = value`,
1314+
Series-vs-DataFrame narrowing). PR #2560 added a pyright config that points
1315+
at `delphi/.venv` but did NOT set any rule overrides, so default-mode
1316+
`reportArgumentType` / `reportIndexIssue` errors surface for valid pandas code.
1317+
Verified pre-existing on edge HEAD (not introduced by PR 14a).
1318+
1319+
Handoff written: `~/polis/HANDOFF_PYRIGHT_PANDAS_STUBS.md`. Do NOT just turn
1320+
off rules globally — investigate pandas-stubs version, community patterns,
1321+
targeted ignores. Tracked as Claude task #8.
1322+
1323+
### What's Next
1324+
1325+
PR 14a unblocks (in stack order):
1326+
1. **D10** — Rep comment selection. Research agent already produced a fix
1327+
proposal (this session). Helpers `passes_by_test`, `beats_best_by_test`,
1328+
`beats_best_agr` go top-level in `repness.py`; reduce structure uses
1329+
`df.to_dict('records')` iteration with mutable `{sufficient, best, best_agree}`
1330+
state. Boundary cases identified for synthetic test fixtures.
1331+
2. **D11** — Consensus selection. Research agent produced a fix proposal:
1332+
needs new `consensus_stats_df(vote_matrix_df) -> pd.DataFrame` (whole-data,
1333+
not per-group), plus rewrite of `select_consensus_comments_df` with the
1334+
`{'agree': [...], 'disagree': [...]}` output shape (top 5 each).
1335+
`conv_repness` must grow `mod_out` kwarg.
1336+
3. **D12** — Comment priorities. Research agent produced a fix proposal:
1337+
Clojure source at `conversation.clj:311-330,341-352,648-679`;
1338+
`pca.clj:167-178`. Needs new `pca_project_cmnts`, `comment_extremity`,
1339+
`importance_metric`, `priority_metric` in Python. `meta_tids` shape mismatch
1340+
(Python set vs Clojure map) flagged.
1341+
4. **PR 14b** — Backfill missing blob injection tests (D7 metric, D8 finalize,
1342+
full stats-stage injection).
1343+
5. **Goldens** — Re-record `vw` and `biodiversity` (sklearn KMeans seeding
1344+
decision pending — see `delphi/scratch/COPILOT_MATH_QUESTIONS.md`).
1345+
6. **PR 14c** — Readability refactor of `compute_group_comment_stats_df`,
1346+
using the deleted scalar code (retrievable via `git show`) as the
1347+
readability reference. Research agent produced a clean split proposal:
1348+
`_build_group_comment_index` (plumbing) + `_compute_per_group_stats`
1349+
(math) + 5-line orchestrator.

delphi/docs/PLAN_DISCREPANCY_FIXES.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ 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 |
3132
| PR 8 (D10) | — (WIP) || Fix D10: rep comment selection — **NEEDS REWORK** |
3233
| PR 9 (D11) | — (WIP) || Fix D11: consensus selection — **NEEDS REWORK** |
3334
| PR 10 (D3) | — (WIP) || Fix D3: k-smoother buffer — **NEEDS REWORK** |

0 commit comments

Comments
 (0)