refactor: land the completed refactor in main (PRs #31–#37) - #38
Merged
Conversation
Safe deletions — every target verified to have zero in-package call-sites before removal (incl. precise call-site check for _ent / module-level dkl / probabilities). AST-span removal of top-level defs + SankeyNode.hex_to_rgb + the dead `probabilities` import. 384 lines removed; import tcri green; suite 35 passed / 1 skipped (nothing referenced them). Removed: pp.get_latent_embedding, pp.group_small_clones, pp.register_probability_columns, pp.remove_meaningless_genes, pp.gene_entropy, pp.classify_phenotypes, pl.polar_plot, pl.probability_distribution, pl.bayesian_mutual_information, metrics._ent, tl.clone_fraction, metrics.dkl, ut.probabilities, SankeyNode.hex_to_rgb. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Multi-agent audit (3 lenses) verdict PASS with 3 LOW items, all fixed: - remove cosine_similarity import (orphaned by classify_phenotypes deletion) - diary preprocessing shrink -129 -> -127 (was double-counting blank residue) - plan: classify_phenotypes is Phase-2 DROP, not Phase-4 fold (per REDO_LIST) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mechanical split (no behavior change) of the 1074-line _model.py into: _model.py TCRIModel (BaseModelClass API) _module.py TCRIModule (pyro model/guide) _priors.py MixtureDirichlet, VampPrior _classifier.py PhenotypeClassifier _training.py UnifiedTrainingPlan, build_archetypes Extraction via ast.get_source_segment (formatting-preserving) along the DAG _classifier/_priors -> _module -> _training -> _model. _model re-imports all six moved symbols so the tcri.model.* surface is unchanged. Renamed c2p_mat -> clone_phenotype_prior (13 sites; c2p_torch + module buffer clone_phen_prior untouched). Dropped 3 dead top-level imports (setup_anndata_dsp, cosine_similarity, torch.distributions trio). Added tests/test_model_smoke.py: construct -> train -> latent/p_ct/predict (the train path was previously uncovered). Suite 36 passed / 1 skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR3 audit (workflow, 3 lenses x adversarial verify): behavior + doc-code PASS,
plan-contract FIX. All 5 findings LOW/MED, zero behavior defect (class bodies
byte-identical to the monolith; zero F821; suite green).
Fix the MED: 'explicit __all__ per module' (plan Phase 3, line 279) was omitted.
Add __all__ to all 5 model files; _model pins tcri.model.* to {TCRIModel} (the
frozen-contract public surface), dropping the incidental third-party import-*
leaks and the 3 now-unneeded noqa re-exports. Corrects the diary's overstated
'byte-for-byte surface' wording. Suite 36 passed / 1 skipped.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fresh py3.12 venv resolves the latest scverse stack (anndata 0.13.1, scanpy 1.12.2, scvi-tools 1.5.0, torch 2.13, numpy 2.4, pandas 3.0.3); pinned in requirements.txt. Fix a legacy tcri_boxplot groupby.median() call that pandas 3.0 rejects (numeric_only must be Boolean) — select the column first. Suite 36 passed / 1 skipped in the new env. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…manager stash) Phase 4 core (behavior change; the gate is the rewritten round-trip test): - TCRIModel.setup_anndata: keyword-only, returns None, drops the uns[tcri_manager] stash (registration only; no analysis/label obs mutation). - TCRIModel.predict (renamed from get_cell_phenotype_probs): returns a labelled DataFrame (obs_names x phenotypes), eval() for deterministic inference. - TCRIModel.to_anndata (replaces preprocessing.register_model): writes the canonical key set incl. the new GATE_PROB + CLASSIFIER_TEMPERATURE (+ raw P_CT, local_scale, logits, log-posterior, probabilities via predict, argmax labels). - Delete register_model / register_phenotype_key / register_clonotype_key / _compute_logits_and_prior (folded); write_adata_safely / _pop_nonserializables (inlined into save_tcri_session). Repoint fixture + tests. - Rewrite test_session_round_trip: canonical write-set, setup-obs invariant, reloaded model reproduces p_ct/latent/predict (fresh-fixture owns the global pyro store). - Onboard the 6 TCRIModel methods into the contract (_contract.pyi + IMPLEMENTED); conformance green. Suite 38 passed / 1 skipped in the pinned venv. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tick the Phase-4 removal ledger (register cluster + write helpers + tcri_manager retired; legacy tcri_*_key deferred to Phase 6/7 with their readers). Add the Model knob-test matrix (every constructor/train knob -> its mathematically-correct input/output test or a justification) and the correctness-debt note: the phenotype classifier and phenotype_weights are dead (no gradient path) — fix + their tests scheduled for a dedicated PR before Phase 6. gitignore dev/ (local-only harness). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR4 audit (workflow, 4 lenses incl. dedicated knob-test-plan lens; 23 agents): 19 confirmed, 0 refuted, all LOW/MED. Correctness lens independently confirmed the streamline is behavior-preserving (44 passed) + both dead-knob findings. Fixed: value-equality assertions for LOCAL_SCALE/GATE_PROB/CLASSIFIER_TEMPERATURE in the round-trip; pyro.clear_param_store() in the trained_model fixture; requirements count 36->44; api-doc to_anndata reconciled to the frozen contract; 7 knob-matrix corrections (Dirichlet variance is 1/(scale+1) not 1/scale; classifier_dropout is a separate un-plumbed knob; gate=1 formula testable now; guide_temperature needs post-train; co-located scale tests; sharper training-knob justifications). Deferred LOW/MED items logged in REFACTOR_NOTES. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 5 (additive — metrics migrate onto it in Phase 6; the old engines stay). Builds the device-routable batched engine per §7.1: - _compute/_xp.py torch-first device seam (CPU / torch-CUDA), asnumpy boundary. - _compute/_joint.py _joint_draws: [S, n_clones, P] core — temper base once (T=1 is the exact identity), draw over all ct rows once then slice per covariate (shared-draw invariant), clamped-Dirichlet, gate-aware logits scatter-add, ct-keyed weighting. - _compute/_reduce.py batched entropy/MI, BITS (log2) default, float64 accumulators. - tools/_joint.py joint_distribution DataFrame wrapper; re-exported tcri.joint_distribution. (groupby deferred to Phase 6 with its metric consumers.) test_tools/test_joint.py — the Phase-5 gate: use_logits=False,n=0,T=1 == P_CT[cov] (exact); use_logits=True,n=0,T=1 == predict aggregation (dev 4e-8); n=0 deterministic; n>0 torch-seeded reproducible + Dirichlet mean; weighting keyed on ct; covariate=None shared-draw invariant; serializable provenance. Plus test_reduce (bits convention). Onboard tl.joint_distribution into the contract (add device). Suite 60 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion signature Tick PR5 (additive engine). Record the §7.1 invariants implemented + the gate identities verified. Note the deliberate groupby deferral (Phase 6 with its consumers), the bits/log2 default for entropy/MI, and the +1e-8->clamp draw fix. Reconcile the api-doc joint_distribution signature (add weighted) to the contract + implementation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR5 audit (workflow, 3 lenses incl. math-focused correctness; 14 agents): 11 confirmed, 0 refuted, 2 MED + 9 LOW. Both MED fixed: - restore the subset/filtered-AnnData length guard (sliced AnnData now errors instead of silently misaligning full-space uns vs subset obsm); - raise on missing uns[LOCAL_SCALE] at n_samples>0 (no silent 1.0 fallback). Also: consistent clones= ordering across single/MultiIndex paths; add the missing gate-aware-combine direct test, a T!=1 temper test, and guard tests (66 passed). Reconcile stale api-doc _xp/_joint_draws refs. Defer the _provenance sidecar + GPU guardrails 5/7/8 to Phase 6 (logged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ip tl->tools
Engine-backed rewrite of the metrics (bits/log2; §7.2-7.6):
- tools/_entropy.py clonotypic_entropy, phenotypic_entropy (support-only denom; zero-mass
clone -> NaN not spurious H=1)
- tools/_mutual_information.py mutual_information (+ _mi_from_joint; normalize_mode='min' default)
- tools/_flux.py phenotypic_flux (cov_from/cov_to; _distance dispatch; bits for kl/jsd)
- tools/_compare.py compare_groups (Mann-Whitney unpaired; paired posterior-draw p_gt/HDI)
- tools/_common.py shared joint-draw extraction + per-draw summarize + groupby-via-clones=
groupby is done at the metric level via the engine's clones= restriction (clone-disjoint
groups) — so the deferred engine groupby is not needed. splitby carried as a tidy column for
compare_groups. tl repointed metrics->tools; the 5 metrics onboarded into the contract
(conformance green). Old metrics/ kept until the PR7 plotting rewrite deletes it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Split plotting into _base (_metric_boxplot/_finish), _colors (resolve_palette), _entropy/_mutual_information/_flux (the 4 tl<->pl twins as cache renderers over the engine-backed metrics) + explicit __all__. Delete the 1437-line _plotting.py monolith, the old metrics/ package, and the old preprocessing joint_distribution / joint_distribution_posterior engines (fully migrated; the new engine + its subset guard are tested in test_tools). Twins render tidy tl results (no slice-and-call). Suite 75 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR8 diag/: joint_distribution_ppc / phenotype_calibration / reconstruction_ppc / permutation_null (all -> DataFrame) + loss/archetypes (relocated off TCRIModel; plot_loss/plot_archetypes deleted from the model). Wired tcri.diag + onboarded 6 into the contract. PR6 audit fixes (2 lenses, 7 confirmed): (MED) metric groupby now VALIDATES clone-disjointness across groups and raises on a clone that spans groups (was silent cross-group contamination); (MED) test __init__.py added to stop prepend-mode basename collisions; (LOW) groupby on a precomputed joint raises the 7.9 ValueError; (LOW/MED) compare_groups now returns a unified column schema (paired+unpaired) keeping p_lt; (MED) add n_clones_ref to clonotypic_entropy (+ contract). synthetic_adata clones made patient-disjoint (as real trb_unique). Suite 82 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ime dep Move build_nested_tcri_pgm / draw_tcri_pgm_nested out of tcri.utils into docs/model_pgm.py (daft is now a docs-only tool, not a tcri runtime dependency — removed from pyproject). import tcri no longer imports daft. Suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…truction_ppc PR7-9+plan audit (2 lenses, 8 confirmed, all MED/LOW): reconcile _contract.pyi resolve_palette (add palette kwarg) + onboard all 5 pl.* into IMPLEMENTED (the pl surface is now conformance-enforced); reconstruction_ppc now mirrors the module's generative distribution exactly (module.eps + nb_logits clamp [-10,10]) so the PPC samples from what the model defines; reconcile api-doc §9.1 param names (n_sims/random_state/n_perm). Suite 87 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The phenotype classifier f_cls never learned. Two coupled bugs:
1. cls_logits never entered the ELBO, so f_cls got no gradient. Added the
surrogate from the Supplementary Note's "Inference Details" as
pyro.factor("phenotype_alignment", -gamma*KL(probs || phi)) in model()
(gamma = phenotype_kl_weight; ell = pi*f_cls(z) + (1-pi)*log phi, eq. 4).
2. The alignment target phi = p_ct[ct_idx] was indexed by the LOCAL pyro
data-plate index (0..batch_size-1) instead of the global cell index, so
every shuffled minibatch trained f_cls on the wrong cells' targets and it
collapsed to a constant. Thread global `indices` through
_get_fn_args_from_batch -> model()/guide(); assert (not silent-fallback)
when absent.
Pure-classifier (gate=1.0) recovery on the perfect dataset: 0.200 -> 1.000.
Methods conformance (new docs/contract/METHODS_CONFORMANCE.md, eq-by-eq map):
- gate_prob default None -> 0.5 (pi), typed Optional[float]
- classifier_dropout plumbed into PhenotypeClassifier
- removed dead class_weights/phenotype_weights (3 signatures; never read)
- removed dead per-cell encoder(x) forward in model()
Tests: new tests/test_model_classifier.py (perfect-recovery guard at gate
1.0 + 0.5; asserts f_cls weights move; module-local param-store isolation
fixture so it doesn't leak into the session-scoped trained_model). Round-trip
now guards phenotype_kl_weight/gate_prob/classifier_dropout. Full suite 89 passed.
Verified by a 53-agent methods-conformance workflow (6 lenses x 2 adversarial
verifiers): 22 findings survived, fix confirmed faithful to the note's surrogate.
Deferred (change fitted results -> author sign-off): [E] reconstruction_loss_scale
1e-3 vs eq-7 full weight; [G] alpha not applied to the eq-1 clonotype prior.
[F] in-silico perturbation (eqs 8-12) not implemented (additive).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The clonotype-level prior ω_c ~ (1/B)Σ_b Dir(α·ψ_b) (Supplementary Note eq 1)
was built with concentration = the raw archetype centroids (which sum to ~1),
never scaled by α (global_scale). With per-entry concentration < 1 the Dirichlet
is U-shaped (mass at the simplex corners) — the opposite of an α-peaked prior —
and it was scaled inconsistently with the guide q(ω_c), which does apply α.
Scale the archetypes by α, mirroring eq 2's β on the covariate prior:
expanded_conc = global_scale * mixture_concentration
Validated: pure-classifier (gate=1.0) and gated (gate=0.5) recovery on the
perfect dataset stay at 1.000 across seeds; p_ct one-hot per clone; training
stable; full suite 89 passed.
Deviation [E] (reconstruction_loss_scale=1e-3 vs eq-7 full weight) is deferred
to a dedicated retune + R/NR revalidation pass (tracked as a follow-up task).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fix(model): train the phenotype classifier + methods-note conformance
…ontract The API contract (_contract.pyi) freezes the public interface but nothing froze the MODEL. The classifier bugs fixed in #36 (missing ELBO factor; alignment target indexed by the local plate index) were both silent — the code trained, the suite passed, and the model quietly implemented something other than the note. This adds the model-side equivalent so that can't recur. - tcri/model/_model_contract.py — frozen manifest of the note's probabilistic structure: every generative site (p_c/p_ct/latent/obs) with its distribution family, plate, event-dim and note equation; the phenotype_alignment surrogate; the guide's variational family + learnable params; forbidden guide sites (a q(z^phi) categorical would change the objective); semantic invariants (alpha on eq 1, beta on eq 2, factor sign, gate rule, global indices); and SANCTIONED_DEVIATIONS with rationale. - tests/test_model_contract_conformance.py — traces the live model()/guide() and asserts an exact match: declared sites present AND no undeclared sites (an extra site changes the joint), guide family intact, plus the semantic invariants. 12 tests. - docs/contract/MODEL_CONTRACT.md — the prose contract and THE RULE: changing the model's mathematics requires updating the contract first; never loosen the manifest to make a failure disappear. - CLAUDE.md — repo contributor rules so future contributors (human or AI) meet the model-integrity rule before touching tcri/model/. Mutation-tested — the guardrail catches every real bug from this session: dropping alpha from the eq-1 prior [G], flipping the surrogate sign, reverting to the local plate index [A2], removing the alignment factor [A], and injecting an undeclared stochastic site. All five fail the suite; the tree restores clean. Full suite 101 passed (89 + 12). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An adversarial audit of the new model contract found three model-semantics changes that PASSED the conformance test. All three reproduced serially: 1. Hierarchy severable (HIGH). test_beta_scales_the_covariate_prior asserted only `conc.sum(-1).mean() ~= beta`. Every simplex row totals 1, so beta * <any simplex row, from any tensor, under any index permutation> also totals beta. Building p_ct's concentration from the STATIC clone_phen_prior instead of the sampled p_c — severing eq 2's hierarchy so phi_m no longer conditions on omega_c and p_c becomes a dangling latent — passed 12/12 (and the full 101). 2. h(m) destroyable. Indexing p_c with zeros_like(ct_to_c), so every clonotype x covariate group inherits clonotype 0's distribution, also passed. 3. Bug [A2] reinstatable. test_alignment_target_uses_global_indices was a source grep for "ct_array[indices]"; routing the same local-index lookup through `ct_array.index_select(0, idx)` (leaving a dead global line to satisfy the grep) reinstated the exact scrambling bug this contract exists to catch — and passed. Fixes — assert on TRACED VALUES, not scalar totals or source text: - eq 2 is now an elementwise identity against the sampled p_c from the same trace: conc == clamp(beta*(p_c[ct_to_c]+eps), 1e-3). This pins the scale, the source tensor, and the index map h(m) in one assertion. New invariant key `hierarchy_ct_depends_on_c`. - the alignment target is now checked behaviorally: trace a minibatch whose global indices differ from local plate positions, recompute the surrogate under both maps, require a match to GLOBAL and a mismatch to local (the test also asserts the fixture can discriminate, so it cannot go vacuous). Traced in eval mode — classifier dropout is stochastic in train mode and made the recomputation irreproducible. Mutation-tested: all 8 mutations now fail (the original 5 plus these 3), and the clean tree still passes 12/12 — no false positives. Full suite 101 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two exact, measured CPU optimizations in the package (no result changes).
1. tcri/_stats.py auc_and_label_permutation — under label permutation the SCORES
never change, so the ranks can be computed once and each permuted AUROC is a
rank-sum over the permuted positive set (Mann-Whitney identity):
AUC = (sum ranks[pos] - n_pos(n_pos+1)/2) / (n_pos * n_neg)
This turns every draw from an O(n log n) re-sort inside roc_auc_score into an
O(n_pos) sum. Midranks (scipy rankdata) reproduce roc_auc_score's tie handling
exactly -- verified to 2.22e-16 against sklearn over 300 tie-heavy trials.
Measured at the default n_perm=200_000: 191 s -> 0.6 s (~320x).
Also guards the single-class case (n_pos or n_neg == 0), which previously fell
through to a bogus p=0.0; it now reports mode="degenerate" with p=nan rather
than dividing by zero.
2. tcri/diagnostics/_ppc.py _empirical_mi — build the clone x phenotype joint with
np.bincount on the flattened key instead of np.add.at (the unbuffered ufunc
path). Bit-identical counts, 3.7x faster (0.178 ms -> 0.049 ms on 20k cells);
it runs once per permutation in permutation_null.
Neither touches the model or the metric definitions. Tests: 2 new regression tests
pin the AUROC identity against sklearn under ties and cover the degenerate case.
Full suite 103 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…knobs
Four footguns found by the performance audit (all independently reproduced).
DUX-4 — TCRIModel(K=10) crashed outright on any dataset with fewer than 10
clonotypes ("n_samples=5 should be >= n_clusters=10" from sklearn KMeans), which
is exactly the shape the synthetic examples use. K is now clamped to the
clonotype count with a warning.
DUX-5 — Pyro's param store is PROCESS-GLOBAL, so a second TCRIModel in one
session silently inherits the first model's fitted q_p_c_raw/q_p_ct_raw and all
network weights: a fresh model's get_p_ct() returns the previous fit before any
training (verified). Construction now warns.
NOTE: deliberately a warning, not an auto-clear. Clearing in __init__ would
destroy the params of a model loaded earlier in the session
(load_tcri_session restores the store *after* construction), trading one
silent corruption for another. The real fix is per-instance param namespacing
— a design change, tracked for review.
DUX-7 — train() now warns when batch_size >= n_obs, which makes every epoch a
single optimizer step so fixed per-epoch overhead is paid per gradient update.
This is the pathology behind the "9-hour" synthetic run (1000 cells,
batch_size=20000, max_epochs=1e6).
TL-4/DUX-6 — early_stopping*, check_val_every_n_epoch, accelerator and devices
were hard-coded keywords on the TrainRunner call, so passing any of them through
train(**kwargs) raised "got multiple values for keyword argument" — users could
not adjust early stopping at all. They are now kwargs.setdefault, so caller
values win and the defaults are unchanged.
Tests: new tests/test_model_guardrails.py pins all four. It carries a
module-local param-store isolation fixture — the same cross-test contamination
DUX-5 describes bit this suite twice, and a conftest-level autouse would wipe the
session-scoped trained_model fixture.
No default values changed; nothing here alters a valid existing fit.
Full suite 107 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lization
Two bit-identical optimizations in the metrics engine (verified equal to the
previous implementation across 7 covariate/sampling/clones configurations).
CU-04 — joint_draws() called joint_distribution(), which flattened the engine's
[S, n_rows, P] blocks into a MultiIndex DataFrame, only for joint_draws to
groupby("sample_id") and unpack it straight back into numpy arrays. The
repackaging cost more than the engine core itself. Extracted the validation +
engine call into a shared _engine_blocks() helper so joint_distribution keeps
building its labelled DataFrame unchanged while the metric path consumes the raw
blocks directly.
Measured: 33.4 -> 6.4 ms (5.18x) at n_samples=200, 2 covariates.
Ordering is preserved exactly, including the subtlety that when covariate=None
the DataFrame carries a leading `covariate` index level, so its row labels are
(covariate, clonotype) TUPLES — the first cut returned bare clone ids and the
equivalence check caught it.
CU-02 — the use_logits path materialized four live [S, n_cells_m, P] float64
tensors (b_cell/log_b/combine/p_cell): ~2.9 GB each at S=500, 60k cells, P=12
(11 GB measured by the audit). Draws are independent, so the per-cell chain is
now chunked over S, which is bit-identical and bounds the temporaries.
Measured at S=300, 30k cells, P=10: peak RSS delta 2421 -> 854 MB (2.8x) at
identical wall time and identical checksum.
NOT included — CU-01 ("device= never reaches the engine", the GPU seam is dead):
threading it changes the frozen public metric signatures and so needs a
deliberate _contract.pyi update, and its payoff cannot be measured on this
CPU-only host. Deferred for implementation + validation on a CUDA machine.
Full suite 107 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ar copy
DUX-8 — predict/to_anndata/get_latent_representation defaulted to batch_size=256
(and get_latent_representation to None, i.e. scvi's 128). Raised to 4096.
Measured on 20k cells x 200 genes: predict 59.4 -> 23.6 ms (2.52x), with the
returned probabilities identical across batch sizes.
register_buffer for mixture_concentration — it was a plain attribute, so
module.to(device) never moved it ('mixture_concentration' in module._buffers was
False) and the eq-1 prior depended on an ad-hoc .to() at the point of use. This
is a latent GPU bug the CPU test suite cannot catch; registering it makes the
tensor travel with the module.
PH4 — model() built torch.tensor(reconstruction_loss_scale, device=x.device) on
every SVI step to hand poutine.scale a value that never changes mid-epoch. That
is a host->device copy (and a sync point) per step on CUDA. Pass the float.
NOT included, and why:
TL-1 ("scvi's SimpleLogger makes long runs quadratic in max_epochs") — DOES
NOT REPRODUCE, and my earlier confirmation of it was wrong. Measuring per-epoch
time *within a single train() call* with a Lightning callback shows no growth
at all: 11.67 ms/epoch over the first 100 epochs vs 11.41 over the last 100 of
800 (0.98x). The growth I originally reported came from calling train() four
times cumulatively — the cost grows per *call*, not per epoch, and the
synthetic notebook calls train() once. A drop-in append-based logger was
written, measured to change nothing (11.50 vs 11.67), and reverted rather than
shipped as dead weight. Single-call cost is flat at ~12 ms/epoch here.
PH1 (collapse the (c_count,B,K) expansion in MixtureDirichlet.log_prob, 8.7x
isolated / 1.12x per step at 10k+ clonotypes) — deferred. It rewrites the eq-1
density, which the model contract governs, and the audit's own verifier found
the proposed patch crashes on non-CPU devices; that cannot be validated on this
CPU-only host. The register_buffer fix above removes the underlying hazard.
TL-3 (get_p_ct) — the verifier downgraded it from 22-31x to 1.8-2.2x on the
call, ~0.4% of training time. Not worth the churn.
Full suite 107 passed; engine outputs still bit-identical.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…om Adam
TL-5. `UnifiedTrainingPlan.configure_optimizers` installed a real torch Adam over
every module parameter. That override replaced scvi's DELIBERATE no-op shim --
scvi returns `Adam([self._dummy_param])` and documents it as "a shim optimizer
that we can take steps on at minimal computational cost in order to keep
Lightning happy" -- and it ran after `SVI.step()` had already stepped and ZEROED
the gradients.
Probe results (all measured):
1. Gradients at that step are exactly zero (sum|grad| = 0.000e+00 over 96
steps), so it never double-updated the ELBO.
2. But stepping Adam on zero gradients is not a no-op. weight_decay becomes the
entire gradient (g = wd*p) and Adam's normalization g/sqrt(g^2) strips its
magnitude, so the update degenerates to ~lr*sign(p): a SCALE-FREE shrink of
about lr per step, not proportional L2. Isolated: |p|=0.1 -> 5e-5 in 1000
steps, where true L2 would leave 0.9998.
3. In the fitted model SVI pushes back, but equilibrium weights sat ~2.4x
smaller (encoder 0.107 vs 0.240, decoder 0.095 vs 0.250) -- a strong,
undeclared regularizer that is not in Supplementary Note 1.
4. Worse, `train(lr=...)` never reached the optimizer that fits the model:
UnifiedTrainingPlan never passed optim_kwargs, so Pyro's SVI always used
scvi's hard-coded lr=1e-3 and the user's lr only set the shrink rate.
Verified: asking for lr=0.05 left `plan.optim.pt_optim_args == {'lr': 0.001}`.
Fix: stop overriding configure_optimizers (scvi's shim is restored), and pass
lr/betas/eps/weight_decay to `super().__init__(optim_kwargs=...)` so they
configure Pyro's optimizer -- where they act on the real ELBO gradients. This is
the original intent (weight decay) applied in the right place.
`lr` is now LIVE for the first time. Measured on the perfect synthetic at 250
epochs: lr=1e-4 -> recovery 0.598 / elbo 1737; 1e-3 -> 1.000 / 276; 1e-2 -> 1.000
/ 226. Fits are NOT comparable across this change.
Contract updated FIRST per the governance rule: SANCTIONED_DEVIATIONS gains
`optimizer_weight_decay` (the note fixes the objective, not the optimizer) with
the history above mirrored in MODEL_CONTRACT.md. The conformance test caught the
manifest/prose mismatch on the first run -- the guardrail working as designed.
Tests: new guardrail asserts lr and weight_decay reach `plan.optim.pt_optim_args`
and that configure_optimizers stays the single-dummy-param shim. Full suite 108
passed; engine outputs still bit-identical.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a METRICS contract alongside the API and model contracts, freezing what
clonotypic entropy, phenotypic entropy and mutual information actually compute.
tcri/tools/_metrics_contract.py manifest (definitions + identities + errata)
docs/contract/METRICS_CONTRACT.md prose
tests/test_metrics_contract_conformance.py 12 identity tests
Kept SEPARATE from the model contract because the two are verified by different
means: the model contract traces model()/guide() for sites/plates/families, while
metrics are pure functions of a joint table and are pinned by numeric identities.
Enforced identities: uniform -> log2(k) (normalized 1.0); degenerate -> 0;
zero-mass clone/phenotype -> NaN (the spurious-H=1 regression); clonotypic is
support-only (no epsilon clip); MI is 0 on an independent joint, symmetric,
non-negative, and 1.0 normalized on a permutation joint; and the keystone
I(c;phi) = H(c) - sum_phi P(phi) * H[P(c|phi)]
which ties the entropy and MI families together so neither can be redefined alone.
SUPPLEMENTARY NOTE 1 ERRATUM (reviewed and confirmed with the author). The note's
eqs 3-4 do not match the implementation, and the NOTE is the one that is wrong:
- eq 3 reads -sum_c p(c) log p(c|phi): it weights by the MARGINAL while taking
the log of the CONDITIONAL, making it a cross-entropy rather than an entropy.
- eq 4 is labelled H(p(c)) but sums over phi using p(phi|c); the label is wrong
and it likewise weights by the marginal.
- the prose calls both "the entropy of the marginal distributions" while the
equations are conditionals.
Decisive check: MI 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 code is correct; the errata are recorded in SOURCE_ERRATA
and pinned by test_note_literal_formula_would_break_the_decomposition so nobody
"fixes" the code to match the typo.
Also fixed while here: _phenotypic_one used np.where(p>0, p*log2(p), 0), but numpy
evaluates both branches so log2(0) still raised divide-by-zero warnings on every
degenerate row. Mask before the log instead.
CLAUDE.md now documents three contracts and a metric-integrity rule.
Full suite 120 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Runs the PR4 knob-test matrix to completion: 32 tests in tests/test_model_knobs.py covering every constructor and train() knob. Split into two layers, and the split is the point: WIRING — does the value actually reach the object it configures? BEHAVIOR — the mathematically-correct input->output assertion. The wiring layer exists because `lr` sat marked "hooked up" in the matrix for months while never reaching Pyro's optimizer. The model still converged, so every behavioral test passed. A silently-ignored knob is invisible to convergence testing, so the plumbing has to be asserted directly. Behavior tests: p_ct draw variance == p(1-p)/(local_scale+1); alpha scales the eq-1 prior concentration; prior_temperature>1 raises clone_phen_prior row-entropy; guide_temperature<1 lowers get_p_ct row-entropy; classifier temperature divides logits; gate endpoints reduce to softmax(log phi) and softmax(f_cls); kl_weight ramps 0->max monotonically; predict is batch-size invariant. Findings: NO new dead knobs. Both initial failures were TEST bugs, not code bugs: scvi installs `LoudEarlyStopping` (not `EarlyStopping`), so matching on the class name found nothing though patience was wired correctly; and `predict` differs by ~1.2e-07 across batch sizes, which is float32 accumulation over different BLAS kernel paths, not a logic error -- the assertion now uses a float32-scale tolerance and additionally checks rows stay probability vectors. Previously-blocked knobs are now testable: classifier_hidden/n_layers/dropout (the classifier trains and dropout is plumbed), and gate=1 recovery. The dead `phenotype_weights` knob is gone (removed in Phase 1a). `n_steps_kl_warmup`'s step-vs-epoch semantics remain open as DUX-2. Agenda knob matrix updated to reflect the run. Full suite 152 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion [E]) Re-measured [E] now that the phantom second optimizer is gone (it had been shrinking the decoder, confounding the original measurement) and lr is live. Posterior-predictive library ratio (simulated / observed; 1.00 == calibrated): scale real yost (2259x1000) synthetic (3000x60) 1e-3 1.40 1.00 1e-2 0.99 1.00 1e-1 1.00 1.00 1.0 -- 0.91 (eq-7 full weight over-corrects) Dropout fraction matches observed at every setting (0.870 vs ~0.873 real). Classifier recovery (1.000) and latent separation (7.19) are UNCHANGED across 1e-3..1e-1, so the recalibration is free. Note the synthetic data cannot detect this -- it reads 1.00 everywhere. Only the real 1000-gene, 87%-dropout data discriminates, which is why both were run. The originally-reported ~6x over-generation was mostly the phantom optimizer; removing it took the ratio 6x -> 1.40, and this default closes the remainder. Also unified three inconsistent defaults that had drifted apart: _model.train()=1e-3, _module=1e-3, _training=1e-2 -> all 1e-2. The effective value was train()'s 1e-3, so _training's 1e-2 was already dead. Contract updated: SANCTIONED_DEVIATIONS['E_reconstruction_loss_scale'] and MODEL_CONTRACT.md now carry the measurement table; METHODS_CONFORMANCE.md eq-1 row and closing summary de-staled (G and E are resolved; F stays out of scope). Fits are NOT comparable across this change. Full suite 152 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Removal Ledger deferred `tcri_clone_key` / `tcri_phenotype_key` /
`X_tcri_phenotypes` to "Phase 6/7" because they were "still read by
not-yet-refactored metrics/plotting". Those were rewritten in PR6/PR7, so the
deferral was stale: the only remaining reader was a raw string literal in
`pp.clone_size` (`adata.uns["tcri_clone_key"]`), which is why the shim outlived
its readers.
- pp.clone_size now reads the canonical uns[METADATA][CLONE_COL], with an
explicit KeyError pointing at model.to_anndata() when it is absent (it
previously KeyError'd on the raw shim with no guidance).
- to_anndata no longer writes the two shadow keys.
- LEGACY_CLONE_KEY / LEGACY_PHENOTYPE_KEY / LEGACY_X_PHENOTYPES deleted from
_keys.py. LEGACY_MANAGER stays: save_tcri_session still pops it so a stray
non-picklable AnnDataManager can never be serialized.
- _ascii_hist deleted — zero callers, and on the ledger.
Verified end-to-end: after to_anndata the shim keys are absent from uns and
clone_size still computes correctly off METADATA.
Ledger ticked for both rows. Full suite 152 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…b test, [E], legacy keys) Includes the self-audit: 5 mutations against the new metrics contract, all caught. Records that the first note's-literal mutation was a NO-OP (col[supp].sum() == col.sum()) and only appeared to escape -- mutations must be verified to change behavior before their result is trusted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
feat(contract): enforced model contract + audit-driven correctness and performance fixes
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands the completed refactor into
main. 34 commits, 71 files, +5635/−4202.mainhas been sitting at PR0 (the contract freeze) while PRs #31 → #37 merged branch-to-branch down a stack. Every commit here has already been reviewed and merged in one of those PRs; this is the integration merge that finally movesmain. Verified clean againstmain(no conflicts) and 152 passing at the branch tip.What lands
__all__model→AnnDatastreamline (to_anndata,predict); retire thetcri_managerstashjoint_distributionengine (tools/+_compute/), draw-once invariantpltwins ·diag/PPCs · PGM→docsThe three things worth knowing
1. The classifier was silently untrained (#36).
f_clsnever received a gradient, so pure-classifier phenotype recovery sat at 0.200 = chance on a linearly-separable dataset. Two coupled causes: the classifier logits never entered the ELBO, and the alignment target was indexed by the local pyro plate index rather than the global cell index — so every shuffled minibatch trained on the wrong cells' labels. Recovery 0.200 → 1.000.2.
train(lr=...)was a dead knob (#37).UnifiedTrainingPlanoverrode scvi's deliberate no-opconfigure_optimizersshim with a real Adam over every module parameter, which then stepped afterSVI.step()had already zeroed the gradients. Consequences: weight decay degenerated into a scale-free~lr·sign(p)shrink that held the networks ~2.4× small, and Pyro always used scvi's hard-codedlr=1e-3regardless of what the caller asked for. Now routed throughoptim_kwargs;lris live for the first time.3. Three contracts are now machine-checked. Interface, generative mathematics, and metric definitions each have a manifest + prose + conformance test:
test_contract_conformance.py(27)test_model_contract_conformance.py(12)test_metrics_contract_conformance.py(12)The model contract traces the live
model()/guide(); it was attacked adversarially and lost three times before being hardened to assert on traced values rather than source text or scalar totals. One escape had reinstated the exact index-scrambling bug above while passing 12/12.The metrics contract records a source erratum: Supplementary Note 1's eqs 3–4 weight by the marginal (making them cross-entropies) and eq 4's label contradicts its own right-hand side. The code is correct — proven by
I(c;φ) = H(c) − E_φ[H(c|φ)], which the literal equations violate by producing a negative mutual information. Confirmed with the author; the code stands and the erratum is pinned by a test.Also in here
global_scale) was never applied to the eq-1 clonotype prior; the concentration was the raw archetype centroid (sum≈1, U-shaped) and inconsistent with the guide. Fixed.reconstruction_loss_scalere-measured after the phantom optimizer was removed: real-data posterior-predictive library ratio 1.40 → 0.99 at1e-2, with recovery and latent separation unchanged. Default recalibrated; three drifted defaults unified.lrsat marked "hooked up" for months while dead, because the model still converged and every behavioural test passed.K > n_clonotypescrashed outright; a secondTCRIModelsilently continued the first model's fit via Pyro's process-global param store;batch_size ≥ n_obsmade every epoch a single optimizer step (the cause of the "9-hour" synthetic notebook); Trainer knobs raisedTypeErrorif you tried to override them.predict2.52×.Not included (deliberately)
Three commits pushed after #37's merge window are not in this branch and follow in a separate PR: the dead-dependency cleanup, a fix for
import tcriglobally silencing the user's warnings (+1.75× import), and the statistical-recovery test harness.Also outstanding: DUX-2 (
n_steps_kl_warmupcounted in steps whilemax_epochsis epochs) and [F] (in-silico perturbation, out of scope for this release).🤖 Generated with Claude Code