From e8d840ecfc1f3eefbbae77555239718ac7f1be42 Mon Sep 17 00:00:00 2001 From: Nicholas Ceglia Date: Mon, 3 Aug 2026 14:59:56 -0400 Subject: [PATCH] docs(contract): enforce the corrected manuscript equations; retire the erratum The manuscript's Entropy section has been corrected: eqs 3-4 now weight by the CONDITIONAL (matching the log) and eq 4's left-hand side reads H(p(phi|c)) instead of the impossible H(p(c)). Both now agree with the implementation exactly. Verified before changing anything (small asymmetric 3x2 joint): eq 3 1.251629 / 1.061278 == _clonotypic_one MATCH eq 4 0.721928 / 1.0 / 0.591673 == _phenotypic_one MATCH eq 5 0.288703 == _mi_from_joint MATCH Added tests that transcribe eqs 2-6 LITERALLY from the manuscript and assert the code computes exactly that. These are now the primary definitional tests; the pre-existing identity tests (uniform -> log2 k, degenerate -> 0, zero-mass -> NaN, support-only, the MI decomposition) stay, but they pin CONSEQUENCES of a formula rather than the formula itself. SOURCE_ERRATA removed from the manifest, __all__, the two note_eq references, the module docstring, METRICS_CONTRACT.md and CLAUDE.md. A short historical note stays in the prose so a reader who finds the old revision is not confused. test_note_literal_formula_would_break_the_decomposition is REFRAMED rather than deleted, as test_marginal_weighting_is_not_an_entropy: marginal-weighting is the natural way to mis-transcribe these equations and it fails two ways at once -- the value can exceed log2(|C|) (impossible for an entropy over |C| outcomes) and it makes the MI decomposition return a NEGATIVE mutual information. Worth keeping as a standing guard independent of any manuscript revision. ONE LIVE DEVIATION, now explicit. Eq 6 defines NMI = I / ((1/2)(H(c)+H(phi))) -- the MEAN denominator -- while tcri defaults to 'min'. On the contract's test joint that is 0.239 (eq 6) vs 0.293 (default), ~23% apart. 'min' is deliberate: the mean denominator scales with log2(C) and so is not comparable across groups with different clone counts. Recorded as SANCTIONED_EXTENSIONS['normalize_mode_default'] and pinned by test_eq6_nmi_is_the_average_denominator, which asserts BOTH that 'average' reproduces eq 6 AND that the default does not -- so the divergence cannot go silent. Anything reproducing the note's benchmark must pass normalize_mode='average'. Metrics contract 12 -> 17 tests. Full suite 169 passed, 3 skipped. Co-Authored-By: Claude Opus 4.8 --- .gitignore | 1 + CLAUDE.md | 12 ++- docs/contract/METRICS_CONTRACT.md | 70 +++++++----- tcri/tools/_metrics_contract.py | 53 ++++----- tests/test_metrics_contract_conformance.py | 118 ++++++++++++++++++++- 5 files changed, 186 insertions(+), 68 deletions(-) diff --git a/.gitignore b/.gitignore index d6c0452..eb4c0d5 100644 --- a/.gitignore +++ b/.gitignore @@ -147,3 +147,4 @@ example/*.pdf # local-only dev harnesses (real-data tests; NOT CI) dev/ +docs/contract/ss-small-team-handbook.md diff --git a/CLAUDE.md b/CLAUDE.md index 911bcc1..0b9e596 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -47,11 +47,13 @@ The conformance test pins numeric identities, the keystone being `I(c;φ) = H(c) − Σ_φ P(φ)·H[P(c|φ)]` — it ties the entropy and MI families together so neither can be redefined alone. -**Note 1's eqs 3–4 are mistranscribed** (they weight by the marginal, making them -cross-entropies, and eq 4's label is wrong). The code is correct — proven by the -decomposition above, which the literal equations violate by producing a *negative* -mutual information. These are recorded in `SOURCE_ERRATA`; do **not** "fix" the code to -match them. +**`normalize_mode` deviates from the note deliberately.** Eq 6 defines +`NMI = I/(½(H(c)+H(φ)))` (the mean denominator, `normalize_mode="average"`), but the +default is `"min"` — the mean denominator scales with `log2(C)` and so is not +comparable across groups with different clone counts. Anything reproducing the note's +benchmark must pass `normalize_mode="average"` explicitly. Recorded in +`SANCTIONED_EXTENSIONS['normalize_mode_default']` and pinned by a test that asserts the +default does *not* equal eq 6, so the divergence cannot go silent. ## Working agreement diff --git a/docs/contract/METRICS_CONTRACT.md b/docs/contract/METRICS_CONTRACT.md index 7a613b8..3db8fa4 100644 --- a/docs/contract/METRICS_CONTRACT.md +++ b/docs/contract/METRICS_CONTRACT.md @@ -16,8 +16,9 @@ by **numeric identities** — uniform → log₂(k), independent → MI 0, and t entropy/MI decomposition. Folding them together would force one mechanism to do a job it is bad at. -Source of truth: **Supplementary Note 1**, "Entropy" section (eqs 2–4) — with the -errata below. +Source of truth: **Supplementary Note 1**, "Entropy" section (eqs 2–6). Each equation +is transcribed literally into the conformance test; the one deliberate divergence +(`normalize_mode`) is documented below. **Governance: update this file and the manifest FIRST, then the code.** A failing conformance test means the *meaning of a published number* changed. Never relax an @@ -73,28 +74,49 @@ different clone counts.** That last one is the keystone: it ties entropy and MI together, so you cannot change one without breaking it. -## Errata in Supplementary Note 1 (the code is correct) - -The note's eqs 3–4, read literally, do **not** match the implementation — and the note -is the one that is wrong. Recorded here so nobody "fixes" the code to match a typo. - -1. **Eq 3** reads `H(p(c|φ)) = − Σ_c p(c) log p(c|φ)` — it weights by the **marginal** - `p(c)` while taking the log of the **conditional**. That is a cross-entropy, not an - entropy. -2. **Eq 4** is labelled `H(p(c))` but its right-hand side sums over φ and uses `p(φ|c)`, - so the label is wrong; it also weights by the marginal. -3. The **prose** introduces both as "the entropy of the marginal distributions", but the - equations are conditionals. - -**Why the code is right.** Mutual information must satisfy -`I(c;φ) = H(c) − E_φ[H(c|φ)]`. On a test joint with true MI **0.288703**: - -- the implemented conditional entropy reproduces it **exactly** (0.288703); -- the note's literal formula yields **−0.345883** — a *negative* mutual information, - which is impossible. - -The literal equations are inconsistent with the note's own MI, so they cannot be what -was intended. `test_note_literal_formula_would_break_the_decomposition` pins this. +## Conformance with the manuscript equations + +The manuscript's Entropy section (eqs 2–6) and the implementation agree. Each equation +is transcribed **literally** from the note in +`tests/test_metrics_contract_conformance.py` and asserted against the code — a stronger +check than the identity tests below, which pin *consequences* of a formula +(uniform → log₂ k, degenerate → 0) rather than the formula itself. + +| eq | manuscript | enforced by | +|---|---|---| +| 2 | `H(p(c,φ)) = −Σ p(c,φ) log p(c,φ)` | `test_eq2_joint_entropy` (via `H(c,φ) = H(c)+H(φ)−I`) | +| 3 | `H(p(c\|φ)) = −Σ_c p(c\|φ) log p(c\|φ)` | `test_eq3_clonotypic_entropy_matches_the_manuscript` | +| 4 | `H(p(φ\|c)) = −Σ_φ p(φ\|c) log p(φ\|c)` | `test_eq4_phenotypic_entropy_matches_the_manuscript` | +| 5 | `I(c,φ) = Σ p(φ,c) log( p(c,φ)/(p(φ)p(c)) )` | `test_eq5_mutual_information_matches_the_manuscript` | +| 6 | `NMI = I / (½(H(c)+H(φ)))` | `test_eq6_nmi_is_the_average_denominator` — **see the deviation below** | + +> **Historical note.** An earlier revision of the note mistranscribed eqs 3–4: both +> weighted by the **marginal** while taking the log of the **conditional** (a +> cross-entropy, not an entropy), and eq 4's left-hand side read `H(p(c))` while its +> right-hand side summed over φ. The code was correct throughout and was left +> unchanged; **the manuscript has since been corrected** and the erratum is retired. +> `test_marginal_weighting_is_not_an_entropy` remains as a standing guard, because +> marginal-weighting is the natural way to mis-transcribe these equations: it fails +> two ways at once — the value can exceed `log₂|C|` (impossible for an entropy over +> `|C|` outcomes), and substituting it into the MI decomposition yields a **negative** +> mutual information, impossible for a KL divergence. + +### The one live deviation: `normalize_mode` + +Eq 6 specifies the **mean** denominator. tcri's default is **`min`**: + +| | denominator | value on the contract's test joint | +|---|---|---| +| eq 6 / `normalize_mode="average"` | `½(H(c)+H(φ))` | **0.238915** | +| tcri default `normalize_mode="min"` | `min(H(c), H(φ))` | **0.293032** | + +`min` is the default because the mean denominator scales with `log₂(C)`, making it +**not comparable across groups with different clone counts** — the blocking issue for +any per-group or per-patient comparison. The two differ materially, so **anything +reproducing the note's benchmark must pass `normalize_mode="average"` explicitly.** +`test_eq6_nmi_is_the_average_denominator` asserts both halves — that `"average"` +reproduces eq 6, and that the default does *not* — so the divergence can never become +silent. ## Sanctioned extensions (the note does not specify these) diff --git a/tcri/tools/_metrics_contract.py b/tcri/tools/_metrics_contract.py index d3d2ada..a7faf8f 100644 --- a/tcri/tools/_metrics_contract.py +++ b/tcri/tools/_metrics_contract.py @@ -12,7 +12,7 @@ ``tests/test_metrics_contract_conformance.py``. Source of truth: **Supplementary Note 1**, "Entropy" section (eqs 2-4) and the mutual -information it defines — with the erratum recorded in ``SOURCE_ERRATA`` below. +information it defines (eqs 2-6). Changing any definition here means changing what the published numbers mean. **Update this manifest and ``docs/contract/METRICS_CONTRACT.md`` FIRST, then the @@ -24,7 +24,6 @@ "LOG_BASE", "METRIC_SPECS", "IDENTITIES", - "SOURCE_ERRATA", "SANCTIONED_EXTENSIONS", "MetricSpec", ] @@ -62,7 +61,7 @@ def __repr__(self): # pragma: no cover - debug aid ), normalizer="log2(#supported clones), or log2(n_clones_ref) when given", empty="NaN when the phenotype column has no positive mass", - note_eq="eq 3 (see SOURCE_ERRATA['eq3_weights_marginal'])", + note_eq="eq 3", ), "phenotypic_entropy": MetricSpec( name="phenotypic_entropy", @@ -75,7 +74,7 @@ def __repr__(self): # pragma: no cover - debug aid ), normalizer="log2(P), P = number of phenotype categories", empty="NaN when the clone row has no positive mass", - note_eq="eq 4 (see SOURCE_ERRATA['eq4_label_and_weights'])", + note_eq="eq 4", ), "mutual_information": MetricSpec( name="mutual_information", @@ -130,36 +129,6 @@ def __repr__(self): # pragma: no cover - debug aid } -#: Errors in the source document, kept explicit so nobody "fixes" the code to match a -#: typo. The code is correct; the note's transcription is not. -SOURCE_ERRATA = { - "eq3_weights_marginal": ( - "Note eq 3 reads H(p(c|phi)) = -sum_c p(c) log p(c|phi) — it weights by the " - "MARGINAL p(c) while taking the log of the CONDITIONAL. That is a " - "cross-entropy, not an entropy. The intended (and implemented) quantity is " - "-sum_c p(c|phi) log p(c|phi)." - ), - "eq4_label_and_weights": ( - "Note eq 4 is labelled H(p(c)) but its right-hand side sums over phi and uses " - "p(phi|c), so the label is wrong; it also weights by the marginal p(phi) " - "rather than the conditional. The intended (and implemented) quantity is " - "-sum_phi p(phi|c) log p(phi|c)." - ), - "prose_says_marginal": ( - "The prose introduces eqs 3-4 as 'the entropy of the marginal distributions', " - "but both equations are conditionals. A marginal entropy would be " - "-sum_c p(c) log p(c)." - ), - "why_the_code_is_right": ( - "Decisive check: mutual information must satisfy " - "I(c;phi) = H(c) - E_phi[H(c|phi)]. On a test joint with true MI 0.288703, the " - "implemented conditional entropy reproduces it exactly, while the note's " - "literal formula gives -0.345883 — a negative mutual information, which is " - "impossible. The literal equations are inconsistent with the note's own MI." - ), -} - - #: Deliberate additions beyond the note. Not deviations from its mathematics — the #: note simply does not specify them. SANCTIONED_EXTENSIONS = { @@ -170,7 +139,21 @@ def __repr__(self): # pragma: no cover - debug aid "normalization": ( "`normalized=True` divides by the maximum-entropy value so results land in " "[0,1] and compare across groups of different size. The note defines only the " - "raw entropies." + "raw entropies for eqs 3-4." + ), + "normalize_mode_default": ( + "DEVIATION FROM eq 6. The note's eq 6 defines " + "NMI = I / ((1/2)(H(c) + H(phi))) -- the MEAN denominator, exposed here as " + "normalize_mode='average'. tcri's DEFAULT is 'min' (I / min(H(c), H(phi))), " + "the coefficient of constraint, because the mean denominator scales with " + "log2(C) and is therefore NOT comparable across groups with different clone " + "counts -- the blocking issue for any per-group or per-patient comparison. " + "The two differ materially (0.293 vs 0.239 on the contract's test joint), so " + "anything reproducing the note's benchmark MUST pass " + "normalize_mode='average' explicitly. Pinned by " + "test_eq6_nmi_is_the_average_denominator, which asserts BOTH that 'average' " + "reproduces eq 6 and that the default does not -- so the divergence can never " + "become silent." ), "n_clones_ref": ( "clonotypic_entropy accepts `n_clones_ref` to FIX the normalizer across groups; " diff --git a/tests/test_metrics_contract_conformance.py b/tests/test_metrics_contract_conformance.py index 52d2f38..58ce5c9 100644 --- a/tests/test_metrics_contract_conformance.py +++ b/tests/test_metrics_contract_conformance.py @@ -31,10 +31,120 @@ def test_manifest_is_complete(): assert MC.LOG_BASE == 2 -def test_source_errata_are_documented(): - """The note's eq 3/4 errors must stay recorded, with the justification.""" - for key in ("eq3_weights_marginal", "eq4_label_and_weights", "why_the_code_is_right"): - assert key in MC.SOURCE_ERRATA and len(MC.SOURCE_ERRATA[key]) > 40 +# ── the manuscript equations, transcribed literally ───────────────────────── +# These are the primary definitional tests: each transcribes an equation from +# Supplementary Note 1 straight from the manuscript and asserts the code computes +# exactly that. Stronger than the identity tests below, which pin *consequences* of +# the formula (uniform -> log2 k, degenerate -> 0) rather than the formula itself. + +def _joint(): + """A small, deliberately asymmetric clone x phenotype count table.""" + return np.array([[4.0, 1.0], [1.0, 1.0], [1.0, 6.0]]) + + +def test_eq2_joint_entropy(): + """Eq 2: H(p(c,phi)) = -sum_{c,phi} p(c,phi) log p(c,phi).""" + J = _joint() + P = J / J.sum() + eq2 = -np.sum(P[P > 0] * np.log2(P[P > 0])) + # the package does not expose joint entropy directly, so assert the identity that + # ties it to the quantities that ARE exposed: H(c,phi) = H(c) + H(phi) - I(c;phi) + p_c, p_ph = P.sum(1), P.sum(0) + h_c = -np.sum(p_c * np.log2(p_c)) + h_ph = -np.sum(p_ph * np.log2(p_ph)) + mi = _mi_from_joint(J, normalized=False) + assert eq2 == pytest.approx(h_c + h_ph - mi, abs=1e-12) + + +def test_eq3_clonotypic_entropy_matches_the_manuscript(): + """Eq 3: H(p(c|phi)) = -sum_{c in C} p(c|phi) log(p(c|phi)). + + Note the weight is the CONDITIONAL p(c|phi), matching the log — an earlier + revision weighted by the marginal p(c), which is a cross-entropy and can exceed + log2(|C|) (see ``test_marginal_weighting_is_not_an_entropy``). + """ + J = _joint() + cols = ["phen_A", "phen_B"] + code = _clonotypic_one(J, cols, normalized=False) + for j, ph in enumerate(cols): + col = J[:, j] + p_c_given_phi = col / col.sum() # p(c|phi) + eq3 = -np.sum(p_c_given_phi * np.log2(p_c_given_phi)) + assert code[ph] == pytest.approx(eq3, abs=1e-12), f"eq 3 mismatch for {ph}" + + +def test_eq4_phenotypic_entropy_matches_the_manuscript(): + """Eq 4: H(p(phi|c)) = -sum_{phi in Phi} p(phi|c) log(p(phi|c)).""" + J = _joint() + ids, cols = ["c0", "c1", "c2"], ["phen_A", "phen_B"] + code = _phenotypic_one(ids, J, cols, normalized=False) + for i, c in enumerate(ids): + row = J[i] + p_phi_given_c = row / row.sum() # p(phi|c) + eq4 = -np.sum(p_phi_given_c * np.log2(p_phi_given_c)) + assert code[c] == pytest.approx(eq4, abs=1e-12), f"eq 4 mismatch for {c}" + + +def test_eq5_mutual_information_matches_the_manuscript(): + """Eq 5: I(c,phi) = sum_{c,phi} p(phi,c) log( p(c,phi) / (p(phi) p(c)) ).""" + J = _joint() + P = J / J.sum() + p_c, p_ph = P.sum(1), P.sum(0) + eq5 = np.sum(P * np.log2(P / np.outer(p_c, p_ph))) + assert _mi_from_joint(J, normalized=False) == pytest.approx(eq5, abs=1e-12) + + +def test_eq6_nmi_is_the_average_denominator(): + """Eq 6: NMI = I / ( (1/2)(H(c) + H(phi)) ) — the MEAN denominator. + + The package exposes this as ``normalize_mode="average"``. Its DEFAULT is + ``"min"``, which is a deliberate deviation (the mean denominator scales with + log2(C) and so is not comparable across groups with different clone counts) — + recorded in ``SANCTIONED_EXTENSIONS['normalize_mode_default']``. This test pins + both: that 'average' reproduces eq 6, and that the default does NOT, so the + divergence can never become silent. + """ + J = _joint() + P = J / J.sum() + p_c, p_ph = P.sum(1), P.sum(0) + mi = np.sum(P * np.log2(P / np.outer(p_c, p_ph))) + h_c = -np.sum(p_c * np.log2(p_c)) + h_ph = -np.sum(p_ph * np.log2(p_ph)) + eq6 = mi / (0.5 * (h_c + h_ph)) + + assert _mi_from_joint(J, normalized=True, mode="average") == pytest.approx(eq6, abs=1e-12) + assert _mi_from_joint(J, normalized=True) != pytest.approx(eq6, abs=1e-6), ( + "the default normalize_mode now equals eq 6 — update the contract deliberately" + ) + + +def test_marginal_weighting_is_not_an_entropy(): + """Weighting by the marginal instead of the conditional is provably wrong. + + Kept as a standing guard rather than an erratum: it is the natural way to + mis-transcribe eqs 3-4, and it fails two ways at once — the value can exceed + log2(|C|) (impossible for an entropy over |C| outcomes), and substituting it into + the MI decomposition yields a NEGATIVE mutual information, which is impossible + for a KL divergence. + """ + J = _joint() + P = J / J.sum() + p_c, p_ph = P.sum(1), P.sum(0) + n_clones = J.shape[0] + + # the marginal-weighted quantity for phenotype A + col = J[:, 0] + wrong = -np.sum(p_c * np.log2(col / col.sum())) + assert wrong > np.log2(n_clones), "expected the bound violation that flags it" + + # and it breaks the decomposition + h_c = -np.sum(p_c * np.log2(p_c)) + expected_wrong = sum( + p_ph[j] * (-np.sum(p_c * np.log2(J[:, j] / J[:, j].sum()))) + for j in range(J.shape[1]) + ) + assert h_c - expected_wrong < 0, "expected a negative (impossible) MI" + assert _mi_from_joint(J, normalized=False) > 0 # ── entropy identities ──────────────────────────────────────────────────────