diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 0000000..bb706bc --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,34 @@ +# Contract ownership. +# +# The three contracts and the source documents they derive from may only be changed with +# an approving review from an owner listed here. Everything else in the repo is open to +# normal review. +# +# ENFORCEMENT: this file is ADVISORY on its own. GitHub only blocks a merge when the +# branch protection rule for `main` has "Require review from Code Owners" enabled. That +# is a repository setting and has to be turned on in the GitHub UI; a CODEOWNERS file +# without it changes nothing except who gets auto-requested for review. + +# --- source documents: upstream of every contract --------------------------------- +/docs/contract/source/ @nceglia @salehis + +# --- machine-checked manifests ---------------------------------------------------- +/tcri/_contract.pyi @nceglia @salehis +/tcri/model/_model_contract.py @nceglia @salehis +/tcri/tools/_metrics_contract.py @nceglia @salehis + +# --- the conformance tests that give the manifests teeth -------------------------- +# Weakening a test is equivalent to weakening the contract, so these carry the same bar. +/tests/test_contract_conformance.py @nceglia @salehis +/tests/test_model_contract_conformance.py @nceglia @salehis +/tests/test_metrics_contract_conformance.py @nceglia @salehis + +# --- the prose twins -------------------------------------------------------------- +/docs/contract/MODEL_CONTRACT.md @nceglia @salehis +/docs/contract/METRICS_CONTRACT.md @nceglia @salehis +/docs/contract/METHODS_CONFORMANCE.md @nceglia @salehis +/docs/contract/tcri_api_and_responsibilities.md @nceglia @salehis + +# --- the governance rule itself --------------------------------------------------- +/CLAUDE.md @nceglia @salehis +/.github/CODEOWNERS @nceglia @salehis diff --git a/CLAUDE.md b/CLAUDE.md index 0b9e596..06568cc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,6 +2,28 @@ Single-cell TCR+RNA information-theory metrics on scvi-tools / pyro / scanpy. +## Source of truth, and who may change it + +**The manuscript is upstream of the contracts.** Sohrab Salehi's Supplementary Note is +ground truth for the model; the metrics document is ground truth for the metrics. Where a +contract disagrees with them, **the contract is wrong**. The documents are the first line: +if one is ambiguous, **ASK** — never infer a definition from what makes the code, a test, +or a benchmark come out right. + +Both are archived in `docs/contract/source/` with their hashes recorded in the metrics +manifest and checked by a test, so a revision is detectable rather than something someone +has to notice. + +**Their equation numbers COLLIDE.** Note 1 numbers eqs 1–12 (generative model, variational +family, ELBO, perturbation). The metrics document independently numbers eqs 2–7 +(entropies, MI, NMI, KL). "eq 3" means the VampPrior in one and the clonotypic entropy in +the other — every reference must name its document. Note 1 contains **no** entropy or MI +definitions at all; a metric citing it is citing the wrong source. + +**Only @nceglia and @salehis may change a contract, a conformance test, or a source +document.** Enforced by `.github/CODEOWNERS`, which requires "Require review from Code +Owners" on the `main` branch protection rule to actually block a merge. + ## The three contracts This repo is governed by three frozen contracts. All are machine-checked; a failing @@ -15,7 +37,7 @@ conformance test means **stop and decide**, not "adjust the contract until it pa ### Model integrity (read before touching `tcri/model/`) -The model implements **Supplementary Note 1** (`tcri_supplementary_methods_04_30_26.pdf`) +The model implements **Supplementary Note 1** (`docs/contract/source/supplementary_note_1_SS_2026-08-03.pdf`) — the source of truth. Changing its mathematics means: adding/removing a stochastic site, changing a distribution family or plate, altering the ELBO or the phenotype surrogate, or changing what a prior is scaled by (α on eq 1, β on eq 2). @@ -64,3 +86,17 @@ default does *not* equal eq 6, so the divergence cannot go silent. - **Never read the `example/` notebooks.** They are disposable *outputs* of the refactor, never an input — no caller census, no "is-it-used" checks. - Run tests with the pinned venv: `MPLBACKEND=Agg .venv/bin/python -m pytest tests/ -q`. + +### Branching (this has gone wrong more than once) + +- **Always branch from a fresh `main`.** `git checkout main && git pull` first. Never + branch off a branch that has an open PR: the second PR then *contains* the first, and it + goes stale the moment the first merges. +- **After a PR merges, return to `main` and pull** before starting the next piece of work. +- **Before pushing to a branch with an open PR**, check it is not behind: + `git fetch && git rev-list --count ..origin/main` must be `0`. If it is not, + rebase — and read what landed on `main` first, because someone else's work may already + cover what you were about to write. +- Duplicated work is the symptom to watch for. The eq 3–4 erratum was addressed in three + separate PRs (#41 closed, #42 and #44 both merged with identical titles) because each was + started from a stale base. diff --git a/docs/contract/METHODS_CONFORMANCE.md b/docs/contract/METHODS_CONFORMANCE.md index 5f76437..adcb9de 100644 --- a/docs/contract/METHODS_CONFORMANCE.md +++ b/docs/contract/METHODS_CONFORMANCE.md @@ -7,7 +7,7 @@ Maps the TCRi generative model in **Supplementary Note 1: Methods for Information theoretic metrics for single cell RNA and T-cell receptor sequencing** -(`tcri_supplementary_methods_04_30_26.pdf`) to the implementation, and records +(`docs/contract/source/supplementary_note_1_SS_2026-08-03.pdf`) to the implementation, and records every known deviation. The PDF is the source of truth; this file is the living conformance record (update it whenever the model changes). @@ -44,8 +44,17 @@ Code: `tcri/model/_module.py` (`TCRIModule.model`/`.guide`), `tcri/model/_priors ## Variational family (eq 6) — `_module.py::guide` -- `q(ω_c) = Dir(λ_c)` — `q_p_c_raw` param → `conc_c_guide = clamp(global_scale * q_p_c_sharp)`. α = `global_scale`. -- `q(ϕ_m) = Dir(λ'_m)` — `q_p_ct_raw` param → `conc_ct_guide = clamp(local_scale * q_p_ct_sharp)`. +- `q(ω_c) = Dir(λ_c)` — `q_p_c_raw` param → `conc_c_guide = clamp(global_scale * q_p_c_sharp)`. ⚠️ **[I]** — see below; λ_c's total is pinned to α, not learned. +- `q(ϕ_m) = Dir(λ'_m)` — `q_p_ct_raw` param → `conc_ct_guide = clamp(local_scale * q_p_ct_sharp)`. ⚠️ **[I]** + +> **These two lines record a correspondence that was never verified, and it is false.** +> `q_p_ct_sharp` is row-normalized before scaling, so the concentration's TOTAL is exactly +> `local_scale` — a fixed constant — whereas the note's notation table gives +> λ'_m ∈ ℝ^P_{>0}, a free variational parameter whose magnitude is learned. Writing +> `Dir(λ'_m)` on one side and `β · (a row summing to 1)` on the other and drawing an arrow +> between them is how this survived: the conformance test traces sites, families, plates and +> event dims, and the *structure* of a concentration is not a traced property. See +> deviation **[I]**. - `q(z_i\|x_i) = N(μ_i, σ_i²)` — `encoder(x, batch)` → `Normal(z_loc, z_scale)`, sampled `"latent"`. - `q(z^ϕ_i\|z_i, ϕ) = Cat(softmax(ℓ_i))` — represented by the surrogate, not an explicit categorical sample. @@ -103,6 +112,43 @@ no code path yet. - **KL warmup + z-only scope.** `UnifiedTrainingPlan` ramps `kl_weight` over `n_steps_kl_warmup`, and it scales only the `latent` (z) KL — the two Dirichlet KLs (`p_c`, `p_ct`) are unscaled. A standard annealing schedule; symmetric (no correctness bug) but not part of eq 7's full-weight KL. - **`num_particles`** on `UnifiedTrainingPlan` is honored only on the enumeration path (`TraceEnum_ELBO`); the default `Trace_ELBO` uses 1 MC particle regardless. +### [I] — guide concentration is pinned where eq 6 specifies a free parameter (OPEN) + +| id | deviation | severity | status | +|---|---|---|---| +| I | `q(ϕ_m)`/`q(ω_c)` concentrations are `scale · (normalized row)`, so their TOTAL is fixed at β/α regardless of how many cells the group has. Note 1 eq 6 + the notation table specify λ'_m, λ_c ∈ ℝ^P_{>0} — free variational parameters with learned magnitude. | MED–HIGH | **open** — needs a contract decision | + +**Consequence.** The posterior cannot concentrate with data: a clone-covariate group with 3 +cells and one with 3,000 get the same posterior width. So every credible interval reported at +`n_samples>0` is prior-set rather than data-informed, and comparisons between groups of very +different size are the worst case. Per-entry concentration is `scale/P` (0.3 at the shipped +`local_scale=3`, P=10), which is below 1 and therefore corner-seeking — measured to inflate +`E[NMI(J)]` by +0.10 at weak coupling versus +0.017 at strong coupling. + +**Why the contract did not catch it.** The manifest was written by reading the code +(2026-07-27) two weeks after the code was refactored into place, and it recorded what the code +does. A spec validated only against the implementation converges on the implementation. The +conformance test then verified agreement between manifest and code — both of which disagreed +with the note. + +**Settled: a straight divergence from a spec that was always clear.** Checked against the +2026-04-30 note — eq 6 and the definition of Λ are word-for-word identical to the 2026-08-03 +version, and the *same* expanded notation table is present in both. Both list `λ_c` and `λ'_m` +as "Dirichlet concentration for q(ω_c)/q(ϕ_m)" with domain **ℝ^P_{>0}**, i.e. vectors, and +members of Λ — the set being optimized. Separately they list `α, β` as scalar "Dirichlet +concentration scales (global, local)", `> 0`, appearing only in the conditioning of +`p(· | x; α, β, {ψ_b}, {u_k})`. + +The code uses the scalar β as the vector's total. That conflates a variational parameter with +a prior hyperparameter — two rows of the same table, distinguished by name and by domain. No +reading of either note makes them the same object, so this is not an ambiguity that was +resolved the wrong way. + +**Expect the benchmark to get worse, not better.** The pinned concentration inflates the +estimate, and that inflation is currently compensating for a separate downward error in the +gate fold. Fixing one side alone moves the net error the wrong way — this must not be judged +by whether NMI error dropped, but by whether the posterior concentrates with cells-per-group. + A/A2/B/C/D/H were fixed in the model PR that introduced this file. **G and E are now resolved too** — α is applied to the eq-1 prior, and `reconstruction_loss_scale` was re-measured and recalibrated to `1e-2` (real-data library ratio 1.40 → 0.99). Both diff --git a/docs/contract/MODEL_CONTRACT.md b/docs/contract/MODEL_CONTRACT.md index 43c3b26..cd966f2 100644 --- a/docs/contract/MODEL_CONTRACT.md +++ b/docs/contract/MODEL_CONTRACT.md @@ -1,6 +1,6 @@ # TCRI Model Contract (FROZEN) -**The model this package implements is Supplementary Note 1** (`tcri_supplementary_methods_04_30_26.pdf`). +**The model this package implements is Supplementary Note 1** (`docs/contract/source/supplementary_note_1_SS_2026-08-03.pdf`). This document is the prose contract; `tcri/model/_model_contract.py` is its machine-checkable form; `tests/test_model_contract_conformance.py` enforces it. diff --git a/docs/contract/source/metrics_2026-08-05.docx b/docs/contract/source/metrics_2026-08-05.docx new file mode 100644 index 0000000..041891d Binary files /dev/null and b/docs/contract/source/metrics_2026-08-05.docx differ diff --git a/docs/contract/source/supplementary_note_1_SS_2026-08-03.pdf b/docs/contract/source/supplementary_note_1_SS_2026-08-03.pdf new file mode 100644 index 0000000..9ebb131 Binary files /dev/null and b/docs/contract/source/supplementary_note_1_SS_2026-08-03.pdf differ diff --git a/tcri/tools/_metrics_contract.py b/tcri/tools/_metrics_contract.py index a7faf8f..2daed7d 100644 --- a/tcri/tools/_metrics_contract.py +++ b/tcri/tools/_metrics_contract.py @@ -11,8 +11,16 @@ entropy/MI decomposition) in ``tests/test_metrics_contract_conformance.py``. -Source of truth: **Supplementary Note 1**, "Entropy" section (eqs 2-4) and the mutual -information it defines (eqs 2-6). +Source of truth +--------------- + +The **manuscript is upstream of this contract.** Where they disagree, the contract is +wrong, not the manuscript. If the manuscript is ambiguous, ASK the authors — do not infer +a definition from what makes the code or a benchmark look correct. + +Two source documents, both archived under ``docs/contract/source/`` (see ``SOURCES``). +**Their equation numbers COLLIDE** — "eq 3" is the clonotypic entropy in one and the +VampPrior in the other — so every ``note_eq`` below names its document explicitly. Changing any definition here means changing what the published numbers mean. **Update this manifest and ``docs/contract/METRICS_CONTRACT.md`` FIRST, then the @@ -22,12 +30,37 @@ __all__ = [ "LOG_BASE", + "SOURCES", "METRIC_SPECS", "IDENTITIES", + "OPEN_QUESTIONS", "SANCTIONED_EXTENSIONS", + "UNIMPLEMENTED", "MetricSpec", ] +#: The upstream documents and which equation numbering each owns. Archived in the repo so +#: a claim can be checked without hunting for a file on someone's desktop, and hashed so a +#: revision fails the build instead of going unnoticed. +SOURCES = { + "METRICS": { + "file": "docs/contract/source/metrics_2026-08-05.docx", + "sha256_16": "2eac587d730d2e5e", + "owns": "eqs 2-7: joint entropy, clonotypic/phenotypic entropy, MI, NMI, KL", + "note": "The authority for everything in this module.", + }, + "NOTE_1": { + "file": "docs/contract/source/supplementary_note_1_SS_2026-08-03.pdf", + "sha256_16": "d71df651af7b6e94", + "owns": "eqs 1-12: generative model, variational family, ELBO, in-silico perturbation", + "note": ( + "Authority for the MODEL contract, not this one. Carries NO entropy or MI " + "definitions -- its only mention of the metric is the benchmark estimand " + "MI(C, phi) = I(C; phi). Do not cite its equation numbers here." + ), + }, +} + #: All entropies/MI are reported in **bits**. The note writes an unspecified ``log``; #: tcri fixes base 2 throughout so entropies read as bits and normalizers are log2(k). LOG_BASE = 2 @@ -61,7 +94,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", + note_eq="METRICS eq 3", ), "phenotypic_entropy": MetricSpec( name="phenotypic_entropy", @@ -74,7 +107,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", + note_eq="METRICS eq 4", ), "mutual_information": MetricSpec( name="mutual_information", @@ -91,7 +124,57 @@ def __repr__(self): # pragma: no cover - debug aid "comparable across groups with different clone counts." ), empty="NaN when the table has no positive mass", - note_eq="the MI defined over the joint in the note's Entropy section", + note_eq="METRICS eq 5 (MI); METRICS eq 6 (NMI, mean denominator)", + ), + "phenotypic_flux": MetricSpec( + name="phenotypic_flux", + formula="D(P(phi|c) at cov_from || P(phi|c) at cov_to), D selected by distance_metric", + per="one value per CLONE (how much that clone's phenotype distribution moved)", + support=( + "Both conditional rows are renormalized over all P phenotypes with an eps " + "floor (1e-12) before the divergence; a clone absent from either condition " + "yields NaN rather than a fabricated distance." + ), + normalizer=( + "None -- flux is a divergence, not a normalized quantity. Units follow the " + "kernel: KL and JS in bits (log2), L1 in [0, 2]." + ), + empty="NaN when the clone has no mass in either condition", + note_eq="METRICS eq 7 (D_KL); the prose defines phenotypic flux AS that divergence", + ), +} + + +#: Defined in the METRICS document but NOT implemented. Recorded so the gap stays visible +#: -- do not add a MetricSpec for code that does not exist. +UNIMPLEMENTED = { + "joint_entropy": ( + "METRICS eq 2: H(p(c,phi)) = -sum_{c,phi} p(c,phi) log p(c,phi). tcri exposes the " + "two conditionals (eqs 3-4) and the MI (eq 5) but has no joint-entropy function; " + "the conformance test reaches it only through the identity H(c,phi) = H(c) + " + "H(phi) - I(c;phi)." + ), +} + + +#: Live disagreements with the METRICS document. NOT sanctioned extensions -- each is a +#: decision someone has to make. A test asserts no key appears in both dicts, so a pending +#: decision cannot be quietly refiled as a feature. +OPEN_QUESTIONS = { + "flux_distance_default": ( + "METRICS eq 7 defines phenotypic flux AS the KL divergence, but phenotypic_flux " + "defaults to distance_metric='l1'. The requested behaviour is KL by default with " + "l1/js still available; the kernels already exist in tcri/_distance.py " + "('kl'/'dkl', 'l1', 'js'/'jsd') and only the default differs." + ), + "posterior_summary_of_a_nonlinear_metric": ( + "At n_samples>0 tcri reports E_s[NMI(J_s)] -- the mean of the per-draw NMI. NMI is " + "nonlinear in the joint, so this is not the NMI of the posterior, and the two " + "differ materially (measured 0.224 vs 0.123 on a benchmark cell). Note 1's " + "benchmark prose says 'we report the posterior mean NMI over 200 posterior draws', " + "which reads as the current behaviour but is ambiguous. ASK the authors which " + "estimand is intended. Affects every metric accepting n_samples>0, not only " + "mutual_information." ), } @@ -155,6 +238,12 @@ def __repr__(self): # pragma: no cover - debug aid "reproduces eq 6 and that the default does not -- so the divergence can never " "become silent." ), + "flux_distance_choices": ( + "METRICS eq 7 defines flux as the KL divergence; tcri additionally offers L1 and " + "Jensen-Shannon via `distance_metric`, since a symmetric or bounded measure is " + "sometimes wanted. Offering them is the extension; which one is the DEFAULT is " + "not -- see OPEN_QUESTIONS['flux_distance_default']." + ), "n_clones_ref": ( "clonotypic_entropy accepts `n_clones_ref` to FIX the normalizer across groups; " "without it each group is normalized by its own supported-clone count, which is " diff --git a/tests/test_metrics_contract_conformance.py b/tests/test_metrics_contract_conformance.py index 58ce5c9..c75e018 100644 --- a/tests/test_metrics_contract_conformance.py +++ b/tests/test_metrics_contract_conformance.py @@ -23,7 +23,8 @@ def test_manifest_is_complete(): """Every public metric is specified, and every spec field is filled in.""" assert set(MC.METRIC_SPECS) == { - "clonotypic_entropy", "phenotypic_entropy", "mutual_information" + "clonotypic_entropy", "phenotypic_entropy", "mutual_information", + "phenotypic_flux", } for name, spec in MC.METRIC_SPECS.items(): for field in ("formula", "per", "support", "normalizer", "empty", "note_eq"): @@ -31,6 +32,46 @@ def test_manifest_is_complete(): assert MC.LOG_BASE == 2 +def test_every_spec_names_its_source_document(): + """Equation numbers COLLIDE between the two source documents — "eq 3" is the clonotypic + entropy in one and the VampPrior in the other — so a bare "eq 3" is ambiguous. This makes + that ambiguity un-shippable.""" + for name, spec in MC.METRIC_SPECS.items(): + assert any(src in spec.note_eq for src in MC.SOURCES), ( + f"{name}.note_eq must name its source document (one of {sorted(MC.SOURCES)}); " + f"got {spec.note_eq!r}" + ) + assert "NOTE_1" not in " ".join(s.note_eq for s in MC.METRIC_SPECS.values()), ( + "Supplementary Note 1 carries no entropy/MI definitions — a metric citing it is " + "citing the wrong document." + ) + + +def test_sources_are_archived_with_a_hash(): + """The upstream documents live in the repo, not on someone's desktop, and the recorded + hash turns a silent revision into a failing build.""" + import hashlib + import pathlib + root = pathlib.Path(__file__).resolve().parents[1] + for key, src in MC.SOURCES.items(): + f = root / src["file"] + assert f.exists(), f"SOURCES[{key!r}] missing from the repo: {src['file']}" + got = hashlib.sha256(f.read_bytes()).hexdigest()[:16] + assert got == src["sha256_16"], ( + f"{src['file']} changed (hash {got}, manifest says {src['sha256_16']}). The " + f"source document is UPSTREAM of this contract — reconcile the manifest to the " + f"new document; do not edit the hash to silence this." + ) + + +def test_open_questions_are_not_quietly_sanctioned(): + """A live disagreement with the source document must not be filed as an 'extension'. + Extensions are things the document does not specify; these are things it does.""" + for key in ("flux_distance_default", "posterior_summary_of_a_nonlinear_metric"): + assert key in MC.OPEN_QUESTIONS and len(MC.OPEN_QUESTIONS[key]) > 40 + assert key not in MC.SANCTIONED_EXTENSIONS + + # ── 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