Repository navigation
Chore/modernise and cleanup - #19
Merged
Merged
Conversation
Cleanup found while auditing the repo after the package consolidation. Dead code, none of it referenced anywhere in src/, tests/ or docs/: - UnifiedPredictPayload, a 24-field dataclass that reads like the predict_step contract but is not: predict_step returns a plain dict. - embedding_payload_to_table, whose docstring claimed it was used by downstream analysis and tests. It was used by neither. - Registry.register_many and SupervisedModule.collect_graph_embeddings. - A `reduced` local in the interpretability pipeline, assigned twice and never read. Left over from the refactor that decoupled clustering from the reduced space; reduction_result still carries it downstream, so behaviour is unchanged. Dependencies: joblib and lightning-utilities were hard module-level imports but undeclared, working only because scikit-learn and lightning happen to pull them in. Added an `sce` extra for rpy2, since SingleCellExperiment/rds input is documented as supported but had no install path. Lint: ruff ignored F401, F841 and E203 globally, hiding 16 unused imports and a dead local. All are fixed, so those three rules are now enforced and cannot regress. E402 and E501 stay ignored, with a comment on why: the E402s are the required rootutils.setup_root()-before-import pattern. ruff target-version was py312 while the package supports >=3.11, so 3.12-only syntax could pass lint and break on the supported floor. environment.yaml was still the Lightning template: it pinned python=3.10, which requires-python = ">=3.11" refuses to install on, so following it produced an env the package could not be installed into. It also pulled torchvision and hydra-optuna-sweeper (neither imported anywhere) and omitted PyG entirely. scripts/schedule.sh still invoked `python src/train.py`, a path that stopped existing when the package moved under src/grass_mil. Also renames a user-facing `cluster_count` column to `niche_count`, and corrects two false claims in repository_map.md: the test workflow is multi-OS but single-Python, and pre-commit has no notebook or docs hooks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI pinned torch 2.4.1 / numpy 1.26 / pandas 2.2 while a plain `pip install .` resolved torch 2.12 / numpy 2.5 / pandas 3.0, so users ran a stack CI never exercised. It did not work: torch 2.6 flipped the default of `torch.load(weights_only=...)` from False to True, and 11 tests fail on any modern torch. CI could not see it, because it pinned 2.4.1. Two fixes were needed: - Our own loaders (precomputed PyG graphs, first-party checkpoints) now pass weights_only=False explicitly. These read artifacts this pipeline wrote; weights_only=True cannot unpickle PyG Data without allowlisting internals that change between releases. - Lightning's checkpoint loader is not ours to change, and it fails on any checkpoint whose hyper_parameters hold an OmegaConf container -- which is every checkpoint this package writes. grass_mil._compat allowlists the ten OmegaConf/container types involved, all inert types whose construction cannot execute code, so the protection still applies to the rest of the file. The workflow moves from conda to pip. The conda pyg channel stopped publishing pytorch-sparse and pytorch-scatter at torch 2.4.0, so staying on conda meant staying on 2.4.0 forever. data.pyg.org publishes prebuilt wheels through torch 2.12, including macosx_11_0_universal2 -- which also removes the source build that broke the macOS runner. `--only-binary :all:` makes a source build fail loudly rather than silently start compiling. Verified locally on the exact CI configuration: 390 passed, 3 skipped, with the only skips being the two GPU tests and one pre-existing fixture limit. The torch-sparse-dependent sampler tests all run, so nothing is dropped. The old pinned stack still passes too. environment.ci-spatial.yaml is deleted; the CI stack is now pinned inline in the workflow via TORCH_VERSION, with a note on why it cannot be raised past 2.12. The packaging job deliberately stays unpinned so a break on the newest release surfaces there first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dataloader The three notebooks were internal smoke tests, and they had stopped working: all of them still imported `from src.*`, two hardcoded an absolute config path from another machine, and one pointed at a `configs/` directory that moved into the package. Nothing referenced them except the docs. They are replaced by `vignettes/`, two worked examples on a public dataset: - 01: squidpy's mibitof (MIBI-TOF colorectal carcinoma, 3 images, 36 markers, 8 cell types) -> manifest + per-sample h5ad -> cellular graphs, with the coord_scale_um and tiling settings explained because getting them wrong silently rescales every downstream distance. - 02: BGRL pretraining (no labels), embedding export, and niche discovery through the interpretability report. Both were executed end to end via nbclient against the modernised stack before committing; they are stored without outputs. The vignettes are explicit about what three images can and cannot support: a correct sample-level split leaves one image per fold and the only genuine bag-level variable is single-class in training, so the supervised step is plumbing rather than a result. The weight is on graph construction, SSL and unsupervised niche discovery, which do stand on their own. Writing them surfaced a bug: SpatialOmicsDataModule implemented train/val/test dataloaders but not predict_dataloader, so `grass-mil-predict` -- a shipped console script -- failed with a Lightning MisconfigurationException against the real datamodule. The predict tests missed it because they use a stub datamodule. It now delegates to the test split, matching how exported tables line up with test metrics. Also removes the three notebook_* experiment configs, whose only consumers were the deleted notebooks. Known limitation, documented inline in vignette 2 rather than worked around: `interpretability.spatial.enabled=true` still fails under the ShaDow sampler. The sampler builds batches without Batch.from_data_list(), so to_data_list() raises inside a bare except, instance_graphs are never collected, and the error surfaces as "payload is missing instance_graphs" -- which points at the payload rather than the cause. The niche analysis does not depend on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g errors Every spatial interpretability plugin -- Moran's I, Ripley, neighbourhood enrichment, filtration curves, tissue graph -- was unreachable from the CLI. The chain: build_spatial_table needs payload.instance_graphs, which predict_step recovers by calling batch.to_data_list(). PyG can only un-batch a Batch that was assembled by Batch.from_data_list(), which records the _slice_dict/_inc_dict bookkeeping. The ShaDow samplers construct their Batch directly and delimit subgraphs with `ptr` instead, so to_data_list() raises. That call sat behind a bare `except Exception: subgraph_data_list = None`, so the failure was silent and surfaced four steps later as "predict payload is missing instance_graphs" -- blaming the payload rather than naming the cause. Two changes: - New leaf module grass_mil.batching. `split_batch_by_ptr` reconstructs the per-subgraph Data objects from `ptr`, and `extract_subgraphs` prefers PyG's own to_data_list() and falls back to it. It lives outside both data/ and models/ because both need it and neither should import the other. - predict_step calls extract_subgraphs instead of to_data_list-behind-a-bare- except. A genuine failure to split now raises where it happens. The splitting logic is not new: _apply_transform_to_each_subgraph already did exactly this, then threw the pieces away after re-collating. That is also why the bug survived -- the only test covering this path passes a transform, and the transform branch ends in Batch.from_data_list(), the one case where to_data_list() works. The new test covers the no-transform branch and fails without the fix. Verified end to end on the vignette data: predict now writes spatial_table.csv (2280 edges, median 15 um), and a report with morans_i and ripley enabled completes. Vignette 2 turns the spatial export on and its "known limitation" note is replaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gnettes `filtration_curves` and `per_niche_cell_type_enrichment` both require one cell-type label per instance and raise without it, so neither could run from the CLI. The export carried per-neighbourhood composition but no single label. The obvious label -- the type of the cell each instance is rooted at -- was not recoverable: only 34% of roots could be found in their own subgraph. The cause is that _normalize_shadow_batch runs twice over the same batch and is not idempotent. _resolve_batch_root_node_ids maps root indices through `n_id`; on the second pass it maps already-global ids through `n_id` again and returns unrelated nodes. split_batch_by_ptr then compounded it by rebasing global ids onto subgraph offsets. With both fixed, roots resolve for 100% of instances. `cell_type` is now the root cell's own type. It is deliberately not a summary of the neighbourhood: summarising would quietly turn cell-level statistics into statistics over neighbourhood labels. An interim version of this commit did exactly that (argmax of composition) and was wrong. Note what the sampler does to any distribution over instances: roots are drawn with inverse-frequency weighting by cell type and with repetition, so rare types are over-represented by design and one cell can root several instances. The exported cell-type distribution reflects the sampling design, not the tissue. docs/interpretability_suite.md now states the unit of analysis for every tier-2 statistic, and that none of them is cell-level in the "every cell exactly once" sense. Vignettes are committed with outputs so they render without being run. Vignette 2 enables the spatial export and the full plugin set; all seven plugins now run. Vignette 1's graph plot drew only every third edge, which made a Delaunay triangulation look like something else -- it now draws every edge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…maries The tier-2 analyses currently run on the instance table: one row per ego-graph, with its cell content reduced to `comp_*` fractions and a single label. That makes them measure relationships between neighbourhood summaries. The reports in leomed_sync_2026_06_16/nsclc_tuning_study measure something different, at two levels: - within each niche, over cells: run_per_cluster_nhood_enrichment pools the member ego-graphs, gives each *cell* the niche of the subgraph it came from, and runs cell-type x cell-type enrichment; run_moran_per_cluster computes Moran's I per cell type over that pooled cell graph; run_filtration_curves counts cells by type per edge-distance threshold. - globally, over niches: run_cluster_self_moran computes autocorrelation of the niche assignment itself across whole regions. Neither is reachable from an instance table, because the per-node cell types are not in it. This adds the missing export: build_cell_tables emits one row per node of every sampled ego-graph (cell_id, instance_id, patch_id, node_id, cell_type, is_root, x, y) and one row per intra-subgraph edge with its distance. Joining instance_id against niche labels gives every cell the niche of its ego-graph, which is what the per-niche analyses need. Cells are pooled with duplicates, as in the legacy: overlapping ego-graphs revisit the same cell and each visit is a row. (patch_id, node_id) identifies a real cell; cell_id does not. Enabled by interpretability.cells.enabled=true, which requires a shadow sampler since only then does the payload carry subgraphs. Verified on the vignette cohort: 17,477 cell rows and 76,008 edges from 1,023 ego-graphs, one root per instance, and the pooled cell-type mix tracks the tissue (Tcell_CD4 0.219 vs 0.220 true) rather than the root-sampling bias that skews any instance-level distribution. Feeding these tables to the existing run_neighborhood_enrichment with id_column="cell_id" reproduces the legacy statistic: within niche 1, Epithelial-Epithelial z=84, Epithelial excluding T cells (-22) and fibroblasts (-20), CD8-CD8 z=25. This is the enabling export only. Wiring the plugins to consume it -- per-niche cell-level scopes for enrichment/Moran/Ripley/filtration, and the global niche-level variants -- is not done yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e structure Completes the port of the two-level design used by the NSCLC reports in leomed_sync_2026_06_16/nsclc_tuning_study. Five plugins, all consuming the cell tables added in the previous commit: - cell_type_enrichment_per_niche: cell-type x cell-type contact enrichment computed separately inside each niche, using only edges whose both endpoints lie in it, plus the differential-vs-Background pass the legacy reports do (run_per_cluster_nhood_enrichment). - cell_type_moran_per_niche: Moran's I of each cell-type indicator per niche, giving the niche x cell-type table (run_moran_per_cluster). - cell_filtration_curves: counts both endpoints of every intra-subgraph edge by cell type as the distance threshold grows (run_filtration_curves). - cell_type_ripley_per_niche: centred cross-L between cell types from the cells' own coordinates, each niche a region. - niche_label_moran: the global counterpart -- autocorrelation of the niche assignment itself, i.e. whether niches form contiguous territories (run_cluster_self_moran). Each cell inherits the niche of the ego-graph it was sampled into, mirroring the legacy `cluster_labels[batch.batch]` step. The numerics are the existing tier-2 primitives; these functions only assemble the right inputs, because being "cell-level" is a property of the table rather than of the estimator. The tests use two 8-cell chains with identical composition (4 A, 4 B) differing only in arrangement -- segregated AAAABBBB versus alternating ABABABAB. An analysis that saw one label per ego-graph could not tell them apart; these report opposite-signed same-type enrichment and Moran's I, which is the distinction the whole change is about. Verified end to end on the vignette cohort: all 12 plugins run from `interpretability/plugins@plugins=full`. Within niche 1, Epithelial-Epithelial z=98 with exclusion of T cells and fibroblasts; Moran's I is highest for Epithelial (0.70) and lowest for CD4 T cells (0.04); niches are contiguous (I = 0.41-0.57). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… cohort Removes neighborhood_enrichment, diff_neighborhood_enrichment, per_niche_cell_type_enrichment, filtration_curves, morans_i and ripley. Each treated a whole ego-graph as one node, so they measured how neighbourhood *labels* relate rather than how cells do: two niches with identical composition but opposite spatial arrangement are indistinguishable to them. The cell-level plugins added in the previous commit measure the intended thing, and niche_label_moran covers the global question. The underlying tier-2 functions stay -- the cell-level plugins are built on them, and they keep their own direct tests. Only the instance-level wrappers go. Removing the differential plugin would have dropped condition comparison entirely, which has no cell-level equivalent, so it is replaced rather than deleted: cell_type_diff_enrichment_by_condition differences cell-type contact enrichment between conditions, joining the condition onto cells the same way the niche label is joined. Eight tests covered the removed wrappers. Five were wrapper-only and are gone; three exercised generic pipeline behaviour through a removed plugin (fail-fast on missing required inputs, the report CLI smoke test) and are rewired onto niche_label_moran, which also needs a spatial table. clustering.params.min_cluster_size now defaults to `auto` = 0.5% of the instances being clustered. A fixed 100 does not transfer between cohorts: it is a sensible floor at ~20k instances and swallows every niche at 2k. Note the converse on small cohorts -- 0.5% of the vignette's 1,023 instances is 5, which shatters the embedding into ~90 niches of a handful of cells, so vignette 2 pins an explicit value and says why. Vignette 2 now exports the cell tables, runs every plugin, and gains a section showing cell-type contact enrichment and per-cell-type Moran's I inside a niche. Re-executed and saved with outputs: 17,482 cells all labelled, two niches of 495 and 527 instances. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ict.yaml Vignette 2 previously attached a supervised head for one epoch purely so `grass-mil-predict` had a checkpoint to consume, and said so. It now fine-tunes the actual MIL model, which is what a user wants to see. The cohort has no usable clinical label, so the target is synthetic: is a patch epithelial-rich, thresholded at the median. It is derived from the cell-type composition the model can see, making it a positive control -- a working pipeline must recover it, and one that cannot is broken. The vignette says this where the target is defined rather than leaving the reader to infer it. Deriving the target per patch rather than per image is what makes the example honest: both classes then occur in every image, so the split stays at the sample level, which is the correct choice given patches from one image share a donor and a batch. Eight epochs from the BGRL checkpoint reach test/acc 0.686 on the held-out image, and the run prints its held-out metrics rather than just an exit code. Switching the vignette to `task=finetune_mil` exposed that `configs/inference/predict.yaml` had the same defaults-ordering bug fixed earlier in train.yaml and eval.yaml: `/task` was merged before `/model`, so the task's per-class widths were discarded and predict built a single-logit head, then failed to load the two-class checkpoint with a size mismatch. My own regression test missed it because ROOT_CONFIGS covered only train and eval; it now covers inference/predict too, and fails on the old ordering. With that fixed, predict exports per-class attention (attention_c0/attention_c1 and their logits) and two instance logits, so the published MIL head is what actually runs through the vignette and margin attribution has real attention to work with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Miscellaneous final bug fixes, documentation updates and modernizing vignettes.