fix(pkg): land #39 in main — warning hijack, import time, recovery harness - #40
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>
fix(pkg): warning hijack + import time; recovery-test harness
|
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
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.
#39 never reached
main. It merged intorefactor/pr6-9at15:30:22, seventeen seconds after #38 merged intomainat15:30:05— so its commits landed in the stack branch instead. This carries them the last hop.mainis currently missing:_preprocessing.pyhad a blanketwarnings.filterwarnings('ignore')at module scope, soimport tcrisilenced every warning in the user's session — including theK-clamp, param-store-reuse, andbatch_size >= n_obsguardrails that refactor: land the completed refactor in main (PRs #31–#37) #38 added. Those guardrails do not work inmainas it stands.import tcriwas 6.65 s, 85% of it intcri.preprocessing, of which 2.9 s was an unused eagerimport umap. Now 3.81 s.gseapy,mpltern), the emptytcri/metrics/dir, and 9 Removal Ledger rows verified + ticked.tcri/datasets/+tests/test_recovery.py— the statistical-recovery harness (the only tests with an accuracy oracle).All four were reviewed in #39; this is purely the merge that moves them to
main. Full suite 164 passed, 3 skipped.