Skip to content

Upgraded qc module - #297

Open
jaspreetishar wants to merge 7 commits into
mainfrom
upgraded_qc_module
Open

Upgraded qc module#297
jaspreetishar wants to merge 7 commits into
mainfrom
upgraded_qc_module

Conversation

@jaspreetishar

Copy link
Copy Markdown
Contributor

No description provided.

@cornhundred

cornhundred commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Review notes and proposed refactor: upgraded_qc_module

First off — the underlying idea here is good: per-ROI QC (density, cell area/circularity, gene assignment rates) is useful and a nice complement to the whole-sample metrics we already have.

Small correctness things to fix first (independent of everything below)

  • qc_segmentation's Xenium branch still unpacks 6 values from _process_xenium_technology, which now returns 8 — this will raise a ValueError on any Xenium run today. Easy fix, just flagging so it doesn't slip through.
  • The commented-out cell_gdf = gpd.GeoDataFrame(geometry=cell_gdf["geometry_micron"]) line means cell_gdf["geometry"] is no longer guaranteed to be micron-space / largest-polygon-only for Xenium, so mean_cell_area would come out in the wrong units once the above is fixed. Worth double-checking against what _process_merscope_technology still does.
  • Minor: transcripts.parquet gets read/renamed twice (once inside _process_xenium_technology, once again right after in qc_segmentation), and the new trx_gdf/transformation_matrix outputs aren't consumed yet — probably just needs the caller wired up.

The bigger question: where should this code live?

Looking at this alongside the rest of the package, I think the qc module doesn't quite have the right data-structure home for what it's doing, and this PR is a good excuse to fix that rather than build further on top of it:

  • The new ROI-level QC logic duplicates NeighborhoodCollection. nbhd/collection.py already has calc_transcript_assignment (reads transcripts.parquet, intersects with ROI polygons, computes assigned/unassigned counts — basically what the new xenium_qc_ROI notebook function does by hand), plus calc_population, calc_signature, and per-ROI area/centroid already derived as standard obs columns. The notebook function reimplements a chunk of this from scratch instead of calling into it.
  • qc_segmentation's per-dataset metrics don't have a home in the obs-table pattern the rest of the package uses. DatasetCollection already gives us "one row per dataset" (with a single dataset just being a one-row collection, exactly as you described), plus the calc_* → write to self.obs pattern via the shared CelldegaCollection base class. qc_segmentation instead reads raw files itself and writes a standalone CSV, sitting outside that pattern entirely.
  • classify_cells / filter_orthogonal_expression / orthogonal_expression_calc are a different concern than QC, honestly — they're marker-gene-based cell classification and cross-segmentation-algorithm comparison, which conceptually fits the "compare across dataset rows" case (multiple segmentation approaches = multiple DatasetCollection rows) better than a qc label.

Proposal

Remove the qc module and fold its responsibilities into the collections that already model the right entity:

Current (qc/__init__.py) Proposed home
qc_segmentation's scalar metrics (assigned-transcript %, mean cell area, transcripts/genes per cell) DatasetCollection.calc_qc_metrics() — writes columns to self.obs, one row per dataset
gene_specific_metrics_df DatasetCollection — new mod["qc_by_gene"] modality, mirroring how calc_signature builds a dataset×gene modality
New ROI-level QC (xenium_qc_ROI) NeighborhoodCollection.calc_qc_metrics() (or similar), reusing calc_transcript_assignment/calc_population instead of a bespoke sjoin
classify_cells, orthogonal_expression_calc DatasetCollection — a cross-row comparison method, since it's inherently about comparing multiple segmentation runs
_process_xenium_technology / _process_merscope_technology / _process_custom_technology Become private per-row loaders called internally by the calc_* methods above

One real prerequisite: DatasetCollection doesn't currently have a data_dir/technology concept per row the way NeighborhoodCollection does — that'd need to be added before calc_qc_metrics() could load raw files itself. Not a blocker, just the first piece of plumbing.

Looking forward to discussing.

Additional Ideas

  • Similarly to breaking down the unassigned transcript proportion into neighborhoods - we can break it down per gene and save this in the DatasetCollection under the gene obs
  • we can potentially calculate local proportion of unassigned transcripts per cell and then bin by cluster. This would also be useful for understanding the relative proximity of cells.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants