fix(pkg): warning hijack + import time; recovery-test harness - #39
Merged
Conversation
Packaging cleanup found while auditing pip-deploy readiness.
DEAD DEPENDENCIES removed (both forced an install for nothing):
- gseapy: declared in pyproject.toml AND requirements.txt but imported nowhere
in the package (only in pre-refactor worktree copies).
- mpltern: imported at tcri/utils/_utils.py:11 and never used -- the ternary
plot that needed it was deleted in PR7. `_utils` loads at `import tcri` time,
so this was a hard import-time dependency for zero benefit; confirmed
`mpltern` no longer enters sys.modules on import.
tcri/metrics/ removed. PR6 deleted its .py files but an empty directory holding
only a .DS_Store survived, so that Removal Ledger row was legitimately unticked.
REMOVAL LEDGER: ticked 9 rows after verifying each of the 20 symbols is genuinely
gone (grepped for a remaining `def` in tcri/). Those rows were stale bookkeeping
from PR6/PR7/PR9, not outstanding work -- except the metrics dir above. The
ledger now has zero real outstanding items.
Verified: `python -m build` produces sdist + wheel, `twine check` PASSES on both,
and the wheel's top level is exactly {tcri, dist-info} with the 7 expected
subpackages. Full suite 152 passed.
NOT included: the CI python-matrix fix (["3.10","3.11"] -> add "3.12", in both
tests.yml and release.yml). pyproject's classifiers already claim 3.12 support and
the dev venv is 3.12, so the package advertises a version CI never tests. The
push was rejected -- updating .github/workflows/ requires the GitHub `workflow`
OAuth scope, which this client does not have. Left for a maintainer push.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two release-blocking problems in tcri/preprocessing/_preprocessing.py, found
while profiling `import tcri` for pip-deploy readiness.
1. THE LIBRARY SILENCED EVERY WARNING IN THE USER'S SESSION.
Module scope carried a blanket `warnings.filterwarnings('ignore')`, and this
module is imported by `import tcri`. Verified: after `import tcri`, a plain
`warnings.warn(...)` produced NOTHING. That means the guardrails added earlier
in this branch -- the K clamp, the param-store-reuse notice, the
batch_size >= n_obs warning -- could never reach a real user, along with any
warning from numpy/pandas/scanpy or the user's own code. A library must not
mutate global warning state it does not own. Removed; confirmed the K-clamp
guardrail is now visible in a plain script.
2. `import tcri` TOOK 6.65 s, 85% OF IT IN THIS MODULE.
-X importtime: tcri 5991 ms, of which tcri.preprocessing 5121 ms, of which
umap 2955 ms (umap -> pynndescent -> numba/llvmlite). The module imported umap
TWICE and never used it -- the ternary/UMAP code that needed it is gone, and
the surviving opt-in path (to_anndata(compute_umap=True)) already imports umap
locally.
The whole import block was pre-refactor debris: numpy x5, pandas x4, torch x5,
umap x2, plus scanpy/pyro/scvi/tqdm/scipy.entropy/softmax/Dirichlet/collections/
datetime/Optional/REGISTRY_KEYS -- for a file that now contains exactly two
functions using only numpy and _keys. Also dropped duplicate ANSI colour
constants and unused _console imports.
Measured: 6.65 s -> 3.81 s (1.75x), and umap no longer enters sys.modules.
Tests: two regression guards -- one asserts a warning is still VISIBLE after
`import tcri` (in a subprocess, since pytest resets filters and would mask this),
one asserts umap is not eagerly imported. Full suite 154 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the missing test tier. The existing 154 tests check STRUCTURE -- contracts,
identities, wiring -- which cannot catch an estimator that is well-formed but
numerically wrong. These check ACCURACY against a known truth.
tcri/datasets/simulate_tcri() implements the semi-synthetic generative story from
Supplementary Note 1 ("Generative Model for Semi-Synthetic Simulations"):
pi ~ Dir, omega_c ~ Dir, z_i ~ Cat(pi), phi_i|z_i ~ Cat(omega[z_i]),
x_i|phi_i ~ Poisson(U_i @ V) with U_i ~ Gamma(program of phi_i)
Because pi and omega are known, I(c;phi) is available in CLOSED FORM. Needs no
real dataset to fit (unlike the original sc_simulator), so it is importable,
seeded and fast.
Two oracles are reported, and the distinction drives honest assertions:
true_* population value implied by (pi, omega) -- the estimand
empirical_* value implied by the REALIZED counts -- what a perfect estimator
returns on THIS sample
They differ by finite-sample noise plus the plug-in estimator's upward bias,
~(C-1)(P-1)/(2N ln2) bits. Both are given under BOTH normalizations, because tcri
defaults to normalize_mode="min" while the note's benchmark used the mean
denominator -- comparing across that difference silently inflates the estimate.
Fast tier (every commit, 10 tests):
- oracle respects MI <= min(H_c, H_phi); both NMIs in [0,1]
- omega_concentration monotonically controls the true MI (averaged over seeds)
- fuzziness changes DIFFICULTY ONLY -- true MI identical to 1e-12
- seed determinism; label noise lowers realized but not population MI
- tl.mutual_information == an INDEPENDENT oracle implementation, raw and both
normalizations (the strongest fast test: agreement is evidence, not tautology)
- the two normalizations are provably not interchangeable
- plug-in bias is upward at small N (documents why equality tests are invalid)
- metamorphic: MI invariant to clone/phenotype relabeling and to uniform
replication of counts
Slow tier (--runslow, 3 tests, 34 s total):
- |empirical - true| falls with N, averaged over 6 seeds
- a FITTED model's MI tracks the true MI across three difficulty levels
- posterior HDI coverage
Measured while designing, not asserted blindly:
- HDI coverage came out 8/8 for a nominal 94% interval (mean width 0.103), with
posterior means close to truth (e.g. 0.2868 vs 0.2825). With 8 replicates that
is evidence of no miscalibration, not proof of 94%; establishing the rate needs
~50+ reps and belongs in benchmarks/. The bar was tightened from >=2/5 (which
would pass at 40% coverage) to >=6/8.
- Single-seed convergence is FLAKY: the same seed gave gap +0.003 at N=5000 but
+0.012 at N=20000. Convergence is therefore asserted on a mean over seeds.
conftest gains --runslow (canonical pytest recipe) so the per-commit suite stays
fast while the accuracy tier stays runnable nightly/pre-release.
Full suite 164 passed, 3 skipped.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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. |
nceglia
added a commit
that referenced
this pull request
Aug 3, 2026
fix(pkg): land #39 in main — warning hijack, import time, recovery harness
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
1 participant
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.
Stacked on #38 (base
refactor/pr6-9). Three commits that were pushed after #37's merge window closed, so they are not in the integration branch — plus the new recovery-test harness.Full suite 164 passed, 3 skipped (the slow tier).
1.
import tcriwas silencing the user's warnings (bbdf87f)tcri/preprocessing/_preprocessing.pycarried a blanketwarnings.filterwarnings('ignore')at module scope, and that module is imported byimport tcri. Verified: after importing tcri, a plainwarnings.warn(...)produced nothing.That meant the guardrails added in #37 — the
Kclamp, the param-store-reuse notice, thebatch_size >= n_obswarning — could never reach a real user, along with any warning from numpy/pandas/scanpy or the user's own code. A library must not mutate global warning state it doesn't own.2.
import tcritook 6.65 s, 85% of it in one module (bbdf87f)-X importtime:tcri5991 ms, of whichtcri.preprocessing5121 ms, of whichumap2955 ms (umap → pynndescent → numba/llvmlite). The module importedumaptwice and never used it — the code that needed it was deleted in PR7, and the surviving opt-in path (to_anndata(compute_umap=True)) already imports it locally.The whole import block was pre-refactor debris: numpy ×5, pandas ×4, torch ×5, umap ×2, plus scanpy/pyro/scvi/tqdm/scipy — for a file that now holds exactly two functions using only numpy and
_keys.Measured: 6.65 s → 3.81 s (1.75×), and umap no longer enters
sys.modules.Both are guarded by regression tests. The warning test runs in a subprocess — pytest resets warning filters, so an in-process test would have masked exactly this bug.
3. Packaging cleanup (
7059573)gseapy— declared inpyproject.tomlandrequirements.txt, imported nowhere.mpltern— imported at import-time, unused since PR7.tcri/metrics/— PR6 deleted its.pyfiles but an empty dir holding a.DS_Storesurvived, which is why that Removal Ledger row was legitimately unticked.Verified:
python -m buildproduces sdist + wheel,twine checkPASSES, and a clean-venv install imports correctly with all namespaces.4. Statistical recovery tests (
0a52544) — the missing test tierThe existing suite checks structure (contracts, identities, wiring); none of it can catch an estimator that is well-formed but numerically wrong. These check accuracy against a known truth.
tcri/datasets/simulate_tcri()implements the note's own semi-synthetic story, so I(c;φ) is available in closed form. It needs no real dataset to fit, so it is importable, seeded and fast.Two oracles are reported, and the distinction drives every assertion:
true_*— the population value implied by(pi, omega): the estimandempirical_*— the value implied by the realized counts: what a perfect estimator returns on this sampleThey differ by sampling noise plus the plug-in estimator's upward bias,
~(C-1)(P-1)/(2N ln2)bits. Both are given under both normalizations, because tcri defaults tominwhile the note's benchmark used the mean denominator — comparing across that difference silently inflates the estimate.Fast tier (10 tests, every commit): oracle bounds;
omega_concentrationmonotonically controls the truth;fuzzinesschanges difficulty only — the truth is identical to 1e-12; seed determinism;tl.mutual_information== an independent oracle implementation in raw and both normalizations; the two normalizations are provably not interchangeable; bias direction; metamorphic invariance to relabeling and count replication.Slow tier (
--runslow, 3 tests, 34 s): MAE falls with N over seeds; a fitted model's MI tracks the true MI across difficulty levels; posterior HDI coverage.Two things measured rather than assumed:
benchmarks/. The bar was tightened from>=2/5(which would pass at 40% coverage) to>=6/8.conftestgains--runslowso the per-commit suite stays fast while the accuracy tier stays runnable nightly.Note:
tcri.datasetsis new public API and is not yet in_contract.pyi— it should be onboarded as part of PR 10 (public API + scverse CI) rather than freezing the surface and immediately amending it.🤖 Generated with Claude Code