Skip to content

Commit b3c3c4f

Browse files
committed
fix(delphi): ns includes PASS votes (Clojure parity, pre-D10)
Bug --- `compute_group_comment_stats_df` in `delphi/polismath/pca_kmeans_rep/repness.py` computed `ns = na + nd` and `total_votes = total_agree + total_disagree`, silently dropping PASS (0) votes from every vote count. Clojure reference: `math/src/polismath/math/repness.clj:56-61, :70`: (defn- count-votes [votes & [vote]] (let [filt-fn (if vote #(= vote %) identity)] (count (filter filt-fn votes)))) ... :ns (fnk [votes] (count-votes votes)) `count-votes` with no `vote` argument uses `identity` as the filter predicate. In Clojure 0 is truthy, so `(filter identity ...)` keeps every non-nil entry — including PASS. Therefore Clojure `ns = na + nd + np` (PASS count). Every downstream metric that consumes `ns` — `pa`, `pd`, `pat`, `pdt`, `ra`, `rd`, `rat`, `rdt`, `agree_metric`, `disagree_metric`, and the upcoming D11 `consensus_stats_df` — was off whenever PASS votes existed. Fix --- Both `total_counts` and `group_counts` now compute the count column via `('vote', 'size')` directly in the pandas groupby aggregation. The frame is `dropna(subset=['vote'])`-filtered upstream, so `size()` counts exactly the non-NaN rows — including PASS (0). This is the Clojure `(count (filter identity votes))` recipe verbatim. Both sites carry a comment citing the Clojure reference. Why D5 BlobInjection didn't catch this -------------------------------------- D5's blob-injection tests pull `(n-success, n-trials)` directly from the Clojure blob's `repness` entries and feed them to `prop_test_vectorized`. They bypass `compute_group_comment_stats_df` entirely, so anything downstream of `ns = na + nd` was invisible. The same gap will recur for every fix whose stage-inputs are built by Python code. Pure-formula tests over a tiny vote matrix are the only RED path. Tests added ----------- `TestNsIncludesPassVotes` in `tests/test_repness_unit.py`: - `test_ns_includes_pass_votes` — single comment, 2 agree / 1 disagree / 2 pass → `na=2, nd=1, ns=5`. - `test_ns_all_pass_column` — all-PASS column → `na=0, nd=0, ns=3`. - `test_ns_mixed_with_nan_only_explicit_votes_count` — NaN never counts; PASS always does. - `test_other_votes_includes_other_group_pass` — `other_votes` (the complement of group `ns`) also includes out-of-group PASS. Suite delta ----------- Pre: 295 passed, 12 skipped, 58 xfailed (PR 14a baseline). Post: 299 passed, 12 skipped, 58 xfailed. Delta = +4 (the new tests). No pre-existing test broke — the existing `TestVectorizedFunctions` fixtures used only AGREE/DISAGREE/NaN and were never sensitive. Cascade to D11 -------------- D11's `consensus_stats_df(vote_matrix_df)` (whole-conversation counterpart) will mirror the same recipe at the conversation level. This fix lands BEFORE D10 in the stack so D11's implementation, when it arrives, starts from the corrected ns semantics. D11 should follow the same pure-formula test pattern. Goldens ------- DEFERRED. Re-recording is gated on sklearn-KMeans-seeding consensus; no golden values shift at the goldens commit until D10/D11/D12 land. commit-id:0761e6c3
1 parent 05dae1d commit b3c3c4f

4 files changed

Lines changed: 202 additions & 5 deletions

File tree

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

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1347,3 +1347,64 @@ 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: ns-PASS fix (2026-06-11)
1352+
1353+
**Context.** While preparing the D10/D11/D12 rework on top of PR 14a, a
1354+
Clojure re-read surfaced a latent bug in `compute_group_comment_stats_df`:
1355+
`ns` (and `total_votes`) were computed as `na + nd`, silently dropping PASS
1356+
votes. Clojure's `:ns` is `(count-votes votes)` (math/repness.clj:56-61,
1357+
:70), which calls `(filter identity votes)`. In Clojure 0 is truthy, so
1358+
PASS (0) counts; only `nil` is filtered out. Therefore Clojure
1359+
`ns = na + nd + np`, and every downstream metric (`pa`, `pd`, `pat`,
1360+
`pdt`, `ra`, `rd`, `rat`, `rdt`, `agree_metric`, `disagree_metric`, plus
1361+
D11's `consensus_stats_df` which will mirror the same recipe at the
1362+
whole-conversation level) was off whenever PASS votes existed.
1363+
1364+
**Why D5 BlobInjection didn't catch it.** D5's blob-injection tests pull
1365+
`(n-success, n-trials)` straight from the Clojure blob's `repness`
1366+
entries and feed them to `prop_test_vectorized`. They bypass
1367+
`compute_group_comment_stats_df` entirely, so the bug downstream of
1368+
`ns = na + nd` was invisible. The same gap will recur for D11 / D12 until
1369+
we ship pure-formula tests that build a tiny vote matrix and assert on the
1370+
counts. Lesson: blob-injection is necessary but not sufficient — every
1371+
formula whose inputs are themselves computed by Python needs at least one
1372+
pure-formula unit test that exercises the input-building code.
1373+
1374+
**TDD cycle.**
1375+
- **BASELINE** — full suite at PR 14a parent: 295 passed, 12 skipped,
1376+
58 xfailed.
1377+
- **RED** — added `TestNsIncludesPassVotes` in `tests/test_repness_unit.py`
1378+
with four pure-formula tests: single-comment mixed AGREE/DISAGREE/PASS,
1379+
all-PASS column, NaN-vs-PASS distinction, two-group `other_votes`
1380+
including out-group PASS. All four failed on the buggy code (4 fails).
1381+
- **GREEN** — in `polismath/pca_kmeans_rep/repness.py`:
1382+
- `total_counts` now computes `total_votes=('vote', 'size')` directly in
1383+
the groupby agg, instead of `total_agree + total_disagree`.
1384+
- `group_counts` now computes `ns=('vote', 'size')` directly, instead of
1385+
`na + nd`.
1386+
- `'size'` on the already-`dropna(subset=['vote'])`-filtered frame counts
1387+
exactly the non-NaN entries — including PASS (0). This is the
1388+
Clojure `(count (filter identity votes))` recipe verbatim.
1389+
- Both sites carry a comment citing repness.clj:56-61, :70 and the
1390+
truthy-0 reasoning.
1391+
- Docstring updated: `ns` now documented as `agree + disagree + PASS`.
1392+
- **FULL SUITE** — 299 passed, 12 skipped, 58 xfailed. Delta = +4
1393+
(exactly the new ns-PASS tests). No existing pre-PR-14a or PR-14a test
1394+
broke. The pre-existing `TestVectorizedFunctions` fixtures use only
1395+
AGREE/DISAGREE/NaN (no PASS), so they were never sensitive to the bug.
1396+
1397+
**Cascade to D11.** D11's plan introduces a `consensus_stats_df(vote_matrix_df)`
1398+
function computing whole-conversation stats (the `:mod-out` Clojure path).
1399+
That function will inevitably mirror the same `na + nd + np` recipe — so
1400+
the ns-PASS fix lands BEFORE D10 in the stack to keep D11's implementation
1401+
clean. D11 should follow the same pure-formula test pattern: build a vote
1402+
matrix with mixed PASS and assert `ns == count of non-NaN cells`.
1403+
1404+
**Goldens.** Stays DEFERRED. Re-recording is gated on
1405+
sklearn-KMeans-seeding consensus (see scratch/COPILOT_MATH_QUESTIONS.md);
1406+
no values shift at the goldens commit until D10/D11/D12 land.
1407+
1408+
**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..@+++++'`.

delphi/docs/PLAN_DISCREPANCY_FIXES.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ This plan's "PR N" labels map to actual GitHub PRs as follows:
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 |
3131
| 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) |
3233
| PR 8 (D10) | — (WIP) || Fix D10: rep comment selection — **NEEDS REWORK** |
3334
| PR 9 (D11) | — (WIP) || Fix D11: consensus selection — **NEEDS REWORK** |
3435
| PR 10 (D3) | — (WIP) || Fix D3: k-smoother buffer — **NEEDS REWORK** |

delphi/polismath/pca_kmeans_rep/repness.py

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,8 @@ def compute_group_comment_stats_df(votes_long: pd.DataFrame,
192192
DataFrame indexed by (group_id, comment) with columns:
193193
- na: number of agrees
194194
- nd: number of disagrees
195-
- ns: number of votes (agrees + disagrees)
195+
- ns: number of votes (agrees + disagrees + PASS, Clojure parity;
196+
see repness.clj:56-61, :70)
196197
- pa: probability of agree (with pseudocount smoothing)
197198
- pd: probability of disagree (with pseudocount smoothing)
198199
- pat: proportion test z-score for agree
@@ -221,11 +222,17 @@ def compute_group_comment_stats_df(votes_long: pd.DataFrame,
221222
# Compute total counts per comment BEFORE filtering to group members
222223
# This matches the old behavior where "other" included ALL participants
223224
# not in the current group (even those not in any cluster)
225+
#
226+
# total_votes counts agree + disagree + PASS, matching Clojure's
227+
# `count-votes` (math/src/polismath/math/repness.clj:56-61, :70).
228+
# `count-votes` called with no `vote` arg uses `identity` as the filter
229+
# predicate; in Clojure 0 is truthy, so PASS (0) votes are kept. NaN
230+
# entries are already dropped above. Use size() to count non-NaN rows.
224231
total_counts = votes_only.groupby('comment').agg(
225232
total_agree=('vote', lambda x: (x == AGREE).sum()),
226233
total_disagree=('vote', lambda x: (x == DISAGREE).sum()),
234+
total_votes=('vote', 'size'),
227235
)
228-
total_counts['total_votes'] = total_counts['total_agree'] + total_counts['total_disagree']
229236

230237
# Now add group column and filter to only group members
231238
votes_with_group = votes_only.copy()
@@ -244,12 +251,18 @@ def compute_group_comment_stats_df(votes_long: pd.DataFrame,
244251
# Get all group IDs
245252
all_group_ids = [group['id'] for group in group_clusters]
246253

247-
# Compute vote counts per (group, comment) for votes from group members
254+
# Compute vote counts per (group, comment) for votes from group members.
255+
#
256+
# ns counts agree + disagree + PASS, matching Clojure's `count-votes`
257+
# (math/src/polismath/math/repness.clj:56-61, :70). `count-votes` with
258+
# no `vote` arg uses `identity` as filter; in Clojure 0 is truthy, so
259+
# PASS (0) votes count. NaN entries were already dropped above. Use
260+
# size() to count non-NaN rows.
248261
group_counts = votes_in_groups.groupby(['group_id', 'comment']).agg(
249262
na=('vote', lambda x: (x == AGREE).sum()),
250263
nd=('vote', lambda x: (x == DISAGREE).sum()),
264+
ns=('vote', 'size'),
251265
)
252-
group_counts['ns'] = group_counts['na'] + group_counts['nd']
253266

254267
# Create full index with all (group, comment) combinations to match old behavior
255268
# Old implementation: for each group, iterate over ALL comments (that have any votes)

delphi/tests/test_repness_unit.py

Lines changed: 123 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
# DataFrame-native vectorized functions
1818
prop_test_vectorized, two_prop_test_vectorized, compute_group_comment_stats_df,
1919
)
20+
from polismath.utils.general import AGREE, DISAGREE, PASS
2021
from polismath.conversation.conversation import Conversation
2122

2223

@@ -334,4 +335,125 @@ def test_compute_group_comment_stats_consistency_with_conv_repness(self):
334335
df_row = stats_df.loc[(gid, tid)]
335336

336337
assert np.isclose(entry['pa'], df_row['pa'], atol=1e-10)
337-
assert np.isclose(entry['pd'], df_row['pd'], atol=1e-10)
338+
assert np.isclose(entry['pd'], df_row['pd'], atol=1e-10)
339+
340+
341+
class TestNsIncludesPassVotes:
342+
"""ns / total_votes must count agree + disagree + PASS (Clojure parity).
343+
344+
Clojure (math/src/polismath/math/repness.clj:56-61, :70):
345+
(defn- count-votes [votes & [vote]]
346+
(let [filt-fn (if vote #(= vote %) identity)]
347+
(count (filter filt-fn votes))))
348+
...
349+
:ns (fnk [votes] (count-votes votes))
350+
351+
`count-votes` is called with no `vote` arg → `filt-fn = identity`. In
352+
Clojure, 0 is truthy, so `(filter identity ...)` keeps every non-nil
353+
entry — including PASS (0). Therefore ns = na + nd + np (PASS count).
354+
355+
Python had ns = na + nd, silently dropping PASS. Every downstream metric
356+
(pa, pd, pat, pdt, ra, rd, rat, rdt, agree_metric, disagree_metric,
357+
consensus stats) was off whenever PASS votes existed. D5 BlobInjection
358+
tests bypassed `compute_group_comment_stats_df` entirely (they feed a
359+
pre-baked stats blob), so the bug was invisible there — pure-formula
360+
tests are the only way to RED it.
361+
"""
362+
363+
def test_ns_includes_pass_votes(self):
364+
"""ns counts AGREE + DISAGREE + PASS, not just AGREE + DISAGREE."""
365+
# 5 ptpts, 1 comment, mixed votes: 2 agree, 1 disagree, 2 pass.
366+
# Clojure ns = count of all non-nil = 5.
367+
# Buggy Python ns = na + nd = 3.
368+
votes_long = pd.DataFrame({
369+
'participant': ['p1', 'p2', 'p3', 'p4', 'p5'],
370+
'comment': ['c1'] * 5,
371+
'vote': [AGREE, AGREE, DISAGREE, PASS, PASS],
372+
})
373+
group_clusters = [{'id': 0, 'members': ['p1', 'p2', 'p3', 'p4', 'p5']}]
374+
375+
stats_df = compute_group_comment_stats_df(votes_long, group_clusters)
376+
row = stats_df.loc[(0, 'c1')]
377+
378+
assert row['na'] == 2
379+
assert row['nd'] == 1
380+
assert row['ns'] == 5, (
381+
f"ns should include PASS (Clojure parity); got {row['ns']}"
382+
)
383+
384+
def test_ns_all_pass_column(self):
385+
"""All-PASS column: na=0, nd=0, ns=3 (not 0)."""
386+
votes_long = pd.DataFrame({
387+
'participant': ['p1', 'p2', 'p3'],
388+
'comment': ['c1'] * 3,
389+
'vote': [PASS, PASS, PASS],
390+
})
391+
group_clusters = [{'id': 0, 'members': ['p1', 'p2', 'p3']}]
392+
393+
stats_df = compute_group_comment_stats_df(votes_long, group_clusters)
394+
row = stats_df.loc[(0, 'c1')]
395+
396+
assert row['na'] == 0
397+
assert row['nd'] == 0
398+
assert row['ns'] == 3, (
399+
f"All-PASS column should still have ns=3 (Clojure parity); "
400+
f"got {row['ns']}"
401+
)
402+
403+
def test_ns_mixed_with_nan_only_explicit_votes_count(self):
404+
"""NaN (unvoted) must NOT count; only explicit AGREE/DISAGREE/PASS do."""
405+
# 6 ptpts on c1: 1 agree, 1 disagree, 2 pass, 2 unvoted (NaN).
406+
# Clojure parity: ns = 4 (the 4 explicit votes). NaN never counts.
407+
votes_long = pd.DataFrame({
408+
'participant': ['p1', 'p2', 'p3', 'p4', 'p5', 'p6'],
409+
'comment': ['c1'] * 6,
410+
'vote': [AGREE, DISAGREE, PASS, PASS, np.nan, np.nan],
411+
})
412+
group_clusters = [{'id': 0, 'members': ['p1', 'p2', 'p3', 'p4', 'p5', 'p6']}]
413+
414+
stats_df = compute_group_comment_stats_df(votes_long, group_clusters)
415+
row = stats_df.loc[(0, 'c1')]
416+
417+
assert row['na'] == 1
418+
assert row['nd'] == 1
419+
assert row['ns'] == 4, (
420+
f"ns must include PASS but exclude NaN; got {row['ns']}"
421+
)
422+
423+
def test_other_votes_includes_other_group_pass(self):
424+
"""`other_votes` = total_votes - ns must include PASS in BOTH halves.
425+
426+
Two groups, one comment. Group 0 votes [AGREE, PASS], group 1 votes
427+
[DISAGREE, PASS]. Total na=1, nd=1, total_votes (Clojure) = 4.
428+
Group 0: na=1, nd=0, ns=2 → other_votes=2 (the group-1 disagree + pass).
429+
Group 1: na=0, nd=1, ns=2 → other_votes=2 (the group-0 agree + pass).
430+
"""
431+
votes_long = pd.DataFrame({
432+
'participant': ['p1', 'p2', 'p3', 'p4'],
433+
'comment': ['c1'] * 4,
434+
'vote': [AGREE, PASS, DISAGREE, PASS],
435+
})
436+
group_clusters = [
437+
{'id': 0, 'members': ['p1', 'p2']},
438+
{'id': 1, 'members': ['p3', 'p4']},
439+
]
440+
441+
stats_df = compute_group_comment_stats_df(votes_long, group_clusters)
442+
443+
g0 = stats_df.loc[(0, 'c1')]
444+
assert g0['na'] == 1
445+
assert g0['nd'] == 0
446+
assert g0['ns'] == 2, f"group 0 ns should include its PASS; got {g0['ns']}"
447+
assert g0['other_votes'] == 2, (
448+
f"group 0 other_votes should include group-1 PASS; "
449+
f"got {g0['other_votes']}"
450+
)
451+
452+
g1 = stats_df.loc[(1, 'c1')]
453+
assert g1['na'] == 0
454+
assert g1['nd'] == 1
455+
assert g1['ns'] == 2, f"group 1 ns should include its PASS; got {g1['ns']}"
456+
assert g1['other_votes'] == 2, (
457+
f"group 1 other_votes should include group-0 PASS; "
458+
f"got {g1['other_votes']}"
459+
)

0 commit comments

Comments
 (0)