diff --git a/delphi/polismath/conversation/conversation.py b/delphi/polismath/conversation/conversation.py index 0a0b34f55..405c2d7be 100644 --- a/delphi/polismath/conversation/conversation.py +++ b/delphi/polismath/conversation/conversation.py @@ -147,41 +147,25 @@ def priority_metric(is_meta: bool, the decay factor `1 + 8 * 2^(-S/5)` lets new (low-S) comments bubble up and fades as more votes accumulate. + History: this mirrored Clojure's #1961 truthy-0 bug (every tid took the + meta branch → all priorities 49; issue #2571) until 2026-07-22. Clojure + HEAD passes a real boolean since #2611 (conversation.clj:686), so the + real branching formula is both the correct AND the parity behavior, in + both engine modes. + Args: is_meta: True for meta comments (treated as constant priority). A, P, S, E: see `importance_metric`. Returns: Squared priority value. - - .. warning:: - **Current behavior (parity-bug mirror):** this function ALWAYS - returns ``META_PRIORITY ** 2`` and ignores ``is_meta`` and - ``A, P, S, E``. It deliberately mirrors a Clojure bug — Clojure - treats meta-tid value 0 as truthy, so every tid takes the meta - branch — for byte-for-byte parity. The branching formula described - above is the *intended* semantics, restored once - https://github.com/compdemocracy/polis/issues/2571 is fixed. See the - ``TODO(clojure-parity-bug)`` in the body below. """ - # TODO(clojure-parity-bug): Clojure (conversation.clj:325) treats meta-tid - # value 0 as TRUTHY in (if is-meta ...), so every tid takes the meta branch. - # We mirror this bug for byte-for-byte Clojure parity. Switch back to - # honoring `is_meta` once the GitHub issue resolves: - # https://github.com/compdemocracy/polis/issues/2571 - # Original semantic-correct code preserved below for reference and future - # restoration. - # - # Clojure-parity-bug-mirror: ALWAYS take the meta branch, ignoring is_meta. - return META_PRIORITY ** 2 - - # Original semantically-correct logic, restore when Clojure bug is fixed: - # if is_meta: - # inner = META_PRIORITY - # else: - # decay_factor = 1 + 8 * (2 ** (-S / 5)) - # inner = importance_metric(A, P, S, E) * decay_factor - # return inner ** 2 + if is_meta: + inner = META_PRIORITY + else: + decay_factor = 1 + 8 * (2 ** (-S / 5)) + inner = importance_metric(A, P, S, E) * decay_factor + return inner ** 2 class Conversation: @@ -1370,6 +1354,10 @@ def recompute(self) -> 'Conversation': prev_base_clusters = getattr(result, 'base_clusters', []) prev_group_clusterings = getattr(result, 'group_clusterings', {}) prev_group_k_smoother = getattr(result, 'group_k_smoother', {}) + # Q2: Clojure's :comment-priorities shadows its current-tick input + # with (:group-votes conv) — the PREVIOUS tick's stored group-votes + # (conversation.clj:658). Captured here, consumed in legacy mode only. + prev_group_votes = getattr(result, 'group_votes', {}) # Compute PCA and projections result._compute_pca(prev_pca=prev_pca) @@ -1385,21 +1373,32 @@ def recompute(self) -> 'Conversation': result._compute_repness() # Compute comment priorities (D12 / PR 11). Needs PCA + group_votes. - result._compute_comment_priorities() + result._compute_comment_priorities(prev_group_votes=prev_group_votes) # Compute participant info result._compute_participant_info() return result - def _compute_comment_priorities(self) -> Dict[Any, float]: + def _compute_comment_priorities( + self, + prev_group_votes: Optional[Dict[str, Any]] = None) -> Dict[Any, float]: """ Compute per-tid comment priorities matching Clojure - `:comment-priorities` (conversation.clj:648-679). + `:comment-priorities` (conversation.clj:656-687). Per-tid: sum A/D/S across all groups → P = S - (A + D) → call `priority_metric(is_meta, A, P, S, E)` where E is the comment - extremity computed from PCA. + extremity computed from the CURRENT tick's PCA. + + Which tick's group-votes feed A/D/S is mode-dependent (Q2): Clojure + shadows its current-tick group-votes input with `(:group-votes conv)` + — the PREVIOUS tick's stored value (conversation.clj:658) — so + 'clojure-legacy' mode uses `prev_group_votes` (empty on the first + tick, matching Clojure's nil). 'improved' mode uses the current + tick's (the sane behavior). Either way the CURRENT tick's group-votes + are stored on `self.group_votes` for the next tick's capture — the + in-memory analogue of Clojure persisting :group-votes in math_main. Stores the result on `self.comment_priorities` and also returns it. TS server `nextComment.ts::getNextPrioritizedComment` consumes this @@ -1449,7 +1448,17 @@ def _compute_comment_priorities(self) -> Dict[Any, float]: # the repness-stage aggregation — tracked in the follow-up issue # "delphi: _compute_comment_priorities recomputes group votes on # every tick". - group_votes = self._compute_group_votes() + current_group_votes = self._compute_group_votes() + # Stored for the NEXT tick's prev capture (Clojure keeps :group-votes + # on the conv / in math_main) — in both modes, like self.pca. + self.group_votes = current_group_votes + if resolve_engine_mode() == ENGINE_MODE_LEGACY: + # Q2: previous tick's group-votes (conversation.clj:658); + # {} on the first tick == Clojure's nil (reduce over nothing + # → A/P/S all 0). + group_votes = prev_group_votes if prev_group_votes is not None else {} + else: + group_votes = current_group_votes priorities: Dict[Any, float] = {} for tid in self.rating_mat.columns: diff --git a/delphi/tests/test_discrepancy_fixes.py b/delphi/tests/test_discrepancy_fixes.py index 8039acf4a..6824ee416 100644 --- a/delphi/tests/test_discrepancy_fixes.py +++ b/delphi/tests/test_discrepancy_fixes.py @@ -1949,35 +1949,18 @@ class TestD12CommentPriorities: """ def test_comment_priorities_exist(self, request, conv, clojure_blob, dataset_name): - """Python should produce comment-priorities matching Clojure. - - Per D12.6: Clojure's `(if 0 ...)` truthiness quirk means every tid - takes the meta branch, so Clojure cold_start priorities are all - META_PRIORITY^2 = 49.0 for vw/biodiversity. Python now mirrors this - bug - (priority_metric returns META_PRIORITY**2 unconditionally), so both - sides should yield identical all-constant 49.0. Spearman is not - meaningful when both sides have zero variance — we instead verify - the constant-value parity directly. + """Python produces REAL (varied) comment-priorities; blob coverage holds. + + History: until 2026-07-22 this asserted the all-49 signature on both + sides (Python mirrored Clojure's #1961 truthy-0 bug, #2571). Clojure + HEAD is fixed (#2611) and Python is un-mirrored, so the pins here are + now: (a) priorities exist and cover the blob's tids; (b) Python's + values are NOT the all-constant bug signature. VALUE parity vs + Clojure is no longer checkable against these stale pre-#2611 blobs — + it is validated by the H-B replay battery against Clojure HEAD + (scripts/certify.py); see also the xfail in + test_legacy_clojure_regression.py::test_comment_priorities. """ - # Per-variant xfail (g5, refined 2026-07-05): known-bad only where - # the Clojure incremental blob has VARIED priorities (no truthy-0 - # bug there), so Python's all-49 mirror can't match. FLI and bg2050 - # incremental blobs carry the all-49 signature and DO match — they - # gate. All cold_start variants gate. Once the Clojure bug (#2571) - # is fixed upstream, drop the Python mirror and this xfail. - _varied_priority_incrementals = ( - 'vw-incremental', 'biodiversity-incremental', - 'bg2018-incremental', 'engage-incremental', - 'pakistan-incremental') - if request.node.callspec.id in _varied_priority_incrementals: - request.applymarker(pytest.mark.xfail( - raises=AssertionError, - strict=False, - reason="D12.6: this Clojure incremental blob has varied " - "priorities (no truthy-0 bug there); Python's " - "all-49 mirror cannot match. See issue #2571.")) - clj_priorities = clojure_blob.get('comment-priorities', {}) check.greater(len(clj_priorities), 0, f"Clojure has {len(clj_priorities)} comment priorities") @@ -1998,32 +1981,17 @@ def test_comment_priorities_exist(self, request, conv, clojure_blob, dataset_nam check.greater(len(common_tids), 0, "Should have common priority tids") tids_sorted = sorted(common_tids) - clj_vals = [clj_p[t] for t in tids_sorted] py_vals = [py_p[t] for t in tids_sorted] - clj_unique = set(clj_vals) py_unique = set(py_vals) - print(f"[{dataset_name}] clj_vals sample: {clj_vals[:5]}, " - f"min={min(clj_vals)}, max={max(clj_vals)}, " - f"unique={len(clj_unique)}") print(f"[{dataset_name}] py_vals sample: {py_vals[:5]}, " f"min={min(py_vals)}, max={max(py_vals)}, " f"unique={len(py_unique)}") - # D12.6 Clojure-parity-bug mirror: both sides should return - # META_PRIORITY**2 = 49.0 for every tid. - META_PRIORITY_SQ = META_PRIORITY ** 2 - check.equal(len(clj_unique), 1, - f"Clojure priorities should be all-constant (bug); got {len(clj_unique)} unique") - check.equal(len(py_unique), 1, - f"Python priorities should be all-constant (bug mirror); got {len(py_unique)} unique") - if len(clj_unique) == 1: - (clj_const,) = clj_unique - check.almost_equal(clj_const, META_PRIORITY_SQ, abs=1e-9, - msg=f"Clojure constant priority should be META_PRIORITY**2={META_PRIORITY_SQ}") - if len(py_unique) == 1: - (py_const,) = py_unique - check.almost_equal(py_const, META_PRIORITY_SQ, abs=1e-9, - msg=f"Python constant priority should be META_PRIORITY**2={META_PRIORITY_SQ}") + # Un-mirrored formula: real data always yields varied priorities. + # All-constant output would mean the #2571 mirror crept back in. + check.greater(len(py_unique), 1, + "Python priorities must be varied (real formula), not " + "the all-constant #2571 mirror signature") class TestD12PriorityExtremityAlignment: @@ -2229,10 +2197,6 @@ def test_priority_metric_meta_constant(self): assert priority_metric(True, 5, 2, 10, 1.5) == META_PRIORITY ** 2 assert priority_metric(True, 0, 0, 0, 0) == META_PRIORITY ** 2 - @pytest.mark.xfail(reason="Clojure parity bug mirror (D12.6): priority_metric always " - "returns META_PRIORITY**2 until upstream Clojure bug resolves. " - "Tests pin the semantically-correct formula and will pass again " - "when we revert the mirror.") def test_priority_metric_non_meta_squared(self): """Non-meta: return = (importance * (1 + 8*2^(-S/5)))^2.""" # A=20, P=3, S=20, E=0 — ref from conversation.clj:337 diff --git a/delphi/tests/test_engine_mode.py b/delphi/tests/test_engine_mode.py index 56c39fb6f..fac7319f3 100644 --- a/delphi/tests/test_engine_mode.py +++ b/delphi/tests/test_engine_mode.py @@ -158,7 +158,16 @@ def _recompute_to_dict(self, monkeypatch, mode): # Pin last_updated so the two runs share a deterministic value. conv.last_updated = 0 result = conv.recompute() - return _strip_volatile(result.to_dict()) + d = _strip_volatile(result.to_dict()) + # ONE documented first-tick exception (Q2, 2026-07-22): Clojure's own + # :comment-priorities reads the PREVIOUS tick's group-votes + # (conversation.clj:658), which is nil on the first tick — so + # Clojure-faithful legacy tick-1 priorities come from zero counts and + # CANNOT equal improved's current-tick-based values. Every other key + # keeps the cold-start invariance guarantee. Legacy tick-1 zero + # semantics are pinned in test_priority_unmirror.py. + d.pop('comment_priorities', None) + return d def test_vw_cold_run_identical_across_modes(self, monkeypatch): improved = self._recompute_to_dict(monkeypatch, ENGINE_MODE_IMPROVED) diff --git a/delphi/tests/test_legacy_clojure_regression.py b/delphi/tests/test_legacy_clojure_regression.py index 3eab53a5d..da73415cb 100644 --- a/delphi/tests/test_legacy_clojure_regression.py +++ b/delphi/tests/test_legacy_clojure_regression.py @@ -312,22 +312,20 @@ def test_comment_priorities(self, request, conversation_data): clojure_output = conversation_data['clojure_output'] dataset_name = conversation_data['dataset_name'] - # Per-variant xfail (g5, refined 2026-07-05): known-bad only where - # the Clojure incremental blob has VARIED priorities (no truthy-0 - # bug there). FLI and bg2050 incremental blobs carry the all-49 - # signature and match Python's mirror — they gate, as do all - # cold_start variants. Drop this once the Clojure bug (#2571) is - # fixed and the Python mirror is removed. - _varied_priority_incrementals = ( - 'vw-incremental', 'biodiversity-incremental', - 'bg2018-incremental', 'engage-incremental', - 'pakistan-incremental') - if request.node.callspec.id in _varied_priority_incrementals: - request.applymarker(pytest.mark.xfail( - raises=AssertionError, strict=False, - reason="D12.6: this Clojure incremental blob has varied " - "priorities (no truthy-0 bug there); Python's " - "all-49 mirror cannot match. See issue #2571.")) + # Un-mirror (2026-07-22): Python computes the REAL priority formula + # (Clojure HEAD fixed #1961 via #2611; the #2571 mirror is removed), + # so exact-value parity against these STALE pre-#2611 reference blobs + # (all-49 signature on every cold_start + FLI/bg2050 incrementals; + # bug-free-Clojure varied values on the rest, but from a different + # warm-start trajectory) is not achievable for ANY variant. Value + # parity vs Clojure HEAD is validated by the H-B replay battery + # (scripts/certify.py). Re-enable this comparison after the blobs + # are regenerated with a fixed-Clojure generator (needs prodclone). + request.applymarker(pytest.mark.xfail( + raises=AssertionError, strict=False, + reason="reference blobs predate the Clojure #2611 priority fix; " + "Python un-mirrored 2026-07-22 — exact-value parity is " + "validated via the H-B replay battery until blob regen")) print(f"\n[{dataset_name}] Testing comment priorities...") diff --git a/delphi/tests/test_priority_unmirror.py b/delphi/tests/test_priority_unmirror.py new file mode 100644 index 000000000..b0d867fe8 --- /dev/null +++ b/delphi/tests/test_priority_unmirror.py @@ -0,0 +1,162 @@ +#!/usr/bin/env python3 +""" +Priority un-mirror (D12.6 → resolved) + Q2 prev-tick group-votes. + +Clojure's #1961 truthy-0 bug (every tid took the meta branch → all priorities +49) was fixed upstream in #2611 (merged 2026-07-18, `(contains? meta-tids +tid)` at conversation.clj:686). Python's `priority_metric` mirrored the bug +(#2571) and must now un-mirror: the real branching formula is both the +correct behavior AND the Clojure-HEAD-parity behavior, in BOTH engine modes. + +Separately (CLOJURE_QUIRKS Q2): Clojure's :comment-priorities node SHADOWS +its current-tick group-votes input with `(:group-votes conv)` — the PREVIOUS +tick's stored value (conversation.clj:658). 'clojure-legacy' mode must do the +same; 'improved' mode keeps the current-tick read (the sane behavior). +""" + +import os +import sys + +import numpy as np +import pytest + +sys.path.append(os.path.abspath(os.path.dirname(__file__))) + +from polismath.conversation.conversation import ( + Conversation, + META_PRIORITY, + importance_metric, + priority_metric, +) +from polismath.pca_kmeans_rep.pca import ( + PCA_IMPL_ENV_VAR, + compute_comment_extremity, + pca_project_cmnts, +) +from polismath.utils.engine_mode import ENGINE_MODE_ENV_VAR + + +N_CMTS = 6 + + +def _bloc_votes(): + votes = [] + for i in range(6): + for t in range(N_CMTS): + votes.append({'pid': f'a{i}', 'tid': f'c{t}', + 'vote': 1.0 if t < 3 else -1.0}) + votes.append({'pid': f'b{i}', 'tid': f'c{t}', + 'vote': -1.0 if t < 3 else 1.0}) + return {'votes': votes} + + +def _tick2_votes(): + """Extra votes that change several tids' A/S totals vs tick 1.""" + return {'votes': [ + {'pid': 'n0', 'tid': f'c{t}', 'vote': 1.0} for t in range(N_CMTS) + ]} + + +def _expected_priorities(conv, group_votes): + """Priorities implied by `group_votes` + `conv`'s CURRENT pca/meta state, + via the same production formula pieces (formula wiring is pinned by the + unit tests below; this pins the DATA-FLOW: which tick's group-votes).""" + center = np.asarray(conv.pca['center']) + comps = np.asarray(conv.pca['comps']) + extremity = dict(zip( + conv.rating_mat.columns, + compute_comment_extremity(pca_project_cmnts(center, comps)))) + out = {} + for tid in conv.rating_mat.columns: + A = D = S = 0 + for gv in group_votes.values(): + v = gv.get('votes', {}).get(tid, {'A': 0, 'D': 0, 'S': 0}) + A += v.get('A', 0) + D += v.get('D', 0) + S += v.get('S', 0) + P = S - (A + D) + out[tid] = float(priority_metric( + tid in conv.meta_tids, A, P, S, float(extremity.get(tid, 0)))) + return out + + +@pytest.fixture +def legacy_mode(monkeypatch): + monkeypatch.delenv(PCA_IMPL_ENV_VAR, raising=False) + monkeypatch.setenv(ENGINE_MODE_ENV_VAR, 'clojure-legacy') + + +@pytest.fixture +def improved_mode(monkeypatch): + monkeypatch.delenv(PCA_IMPL_ENV_VAR, raising=False) + monkeypatch.setenv(ENGINE_MODE_ENV_VAR, 'improved') + + +class TestPriorityMetricUnmirrored: + """The real branching formula, restored (both modes — pure function).""" + + def test_non_meta_uses_importance_times_decay_squared(self): + A, P, S, E = 20, 3, 20, 0.7 + expected = (importance_metric(A, P, S, E) * (1 + 8 * 2 ** (-S / 5))) ** 2 + assert abs(priority_metric(False, A, P, S, E) - expected) < 1e-10 + # And it is NOT the mirror constant. + assert priority_metric(False, A, P, S, E) != META_PRIORITY ** 2 + + def test_meta_still_constant_49(self): + assert priority_metric(True, 20, 3, 20, 0.7) == META_PRIORITY ** 2 + + def test_zero_votes_formula(self): + # A=P=S=0: importance = (1 - 1/2) * (E+1) * (1/2); decay = 9. + E = 0.4 + expected = (0.25 * (E + 1) * 9) ** 2 + assert abs(priority_metric(False, 0, 0, 0, E) - expected) < 1e-10 + + +class TestPrioritiesGroupVotesTick: + """Q2 data-flow: which tick's group-votes feed the priorities.""" + + def test_legacy_uses_prev_tick_group_votes(self, legacy_mode): + conv1 = Conversation('q2').update_votes(_bloc_votes()) + gv1 = conv1._compute_group_votes() + conv2 = conv1.update_votes(_tick2_votes()) + gv2 = conv2._compute_group_votes() + + expected_prev = _expected_priorities(conv2, gv1) + expected_curr = _expected_priorities(conv2, gv2) + # The scenario must actually distinguish the two ticks. + assert any(abs(expected_prev[t] - expected_curr[t]) > 1e-9 + for t in expected_prev), "scenario failed to change A/P/S" + + got = {f'c{k}' if not isinstance(k, str) else k: v + for k, v in conv2.comment_priorities.items()} + for tid in expected_prev: + assert abs(got[tid] - expected_prev[tid]) < 1e-9, ( + f"{tid}: legacy priorities must come from the PREVIOUS " + f"tick's group-votes (Clojure conversation.clj:658)") + + def test_legacy_first_tick_uses_empty_group_votes(self, legacy_mode): + conv = Conversation('q2').update_votes(_bloc_votes()) + # Clojure first tick: (:group-votes conv) is nil → A=P=S=0 for every + # tid; only extremity varies. + expected = _expected_priorities(conv, {}) + got = {f'c{k}' if not isinstance(k, str) else k: v + for k, v in conv.comment_priorities.items()} + for tid in expected: + assert abs(got[tid] - expected[tid]) < 1e-9 + + def test_legacy_stores_group_votes_for_next_tick(self, legacy_mode): + conv1 = Conversation('q2').update_votes(_bloc_votes()) + assert conv1.group_votes == conv1._compute_group_votes() + + def test_improved_uses_current_tick_group_votes(self, improved_mode): + conv1 = Conversation('q2').update_votes(_bloc_votes()) + conv2 = conv1.update_votes(_tick2_votes()) + expected_curr = _expected_priorities(conv2, conv2._compute_group_votes()) + got = {f'c{k}' if not isinstance(k, str) else k: v + for k, v in conv2.comment_priorities.items()} + for tid in expected_curr: + assert abs(got[tid] - expected_curr[tid]) < 1e-9 + + +if __name__ == '__main__': + pytest.main([__file__, '-v'])