Skip to content

Add image QC and transcript QC subworkflow (ported from nf-xenium-processing dev HEAD) - #210

Closed
an-altosian wants to merge 6 commits into
nf-core:devfrom
an-altosian:feat/qc-from-upstream-dev
Closed

Add image QC and transcript QC subworkflow (ported from nf-xenium-processing dev HEAD)#210
an-altosian wants to merge 6 commits into
nf-core:devfrom
an-altosian:feat/qc-from-upstream-dev

Conversation

@an-altosian

@an-altosian an-altosian commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Description

Adds the image QC and transcript QC layer, ported from the internal nf-xenium-processing repository at dev HEAD 5e35cae, onto dev alongside the existing spoQC subworkflow.

This replaces #205. That PR was built from a stale upstream snapshot: its transcript QC was 695 lines against upstream's 1344, its report notebook 333 against 1861, and transcript_stream.py and the threshold config were missing entirely. @nnie-altos and @nmalwinka spent their review time re-deriving issues that current upstream had already fixed. This branch starts fresh from dev and from upstream HEAD, so those four items close by provenance rather than by patching.

Provenance, verified rather than asserted

File Status vs upstream 5e35cae
bin/snr_metrics.py, bin/xenium_image_qc_report.qmd, bin/roi_image_qc_thresholds.yaml, bin/transcript_stream.py byte-identical
bin/image_qc.py 3 diff hunks, no logic lines: a typing import, removal of the xenium_helpers import, and the inlined helper block replacing it
bin/transcript_qc_processing.py, bin/transcript_qc.qmd, bin/transcript_qc_thresholds.yaml upstream + a mechanical molecule → transcript rename

xenium_helpers is dropped entirely: the needed helpers are inlined into each script byte-identically to upstream (AST-compared, 7/7 and 11/11 symbols), so a re-sync stays mechanical. The rename is recorded as an ordered, replayable rule set — replaying it on pristine upstream reproduces the notebook and threshold YAML byte-for-byte.

Review items from #205

Closed by porting from upstream HEAD: 1 under-scaled noise threshold (now scaled_noise_threshold), 2 unbounded host memory (now a streaming reader), 3 figure names not matching contents, 4 unassigned% / per-FoV QV, 10 the report's QV-filtering claim (gone upstream).

Fixed here, each with before/after evidence:

  • 5 --non-gene-prefix was ignored (selection hard-coded to NegControl). Now threaded through, with the script default changed to NegControl so the default reproduces the previous behaviour exactly — wiring it naively would have moved the threshold.
  • 6 --non-gene-prefix / --stain-names had no nargs, so the documented multi-value form exited with argparse error 2.
  • 7 estimate_min_mols_per_cell raised from np.quantile on an empty slice (IndexError, not ValueError). Guarded at the call sites so the vendored helper stays byte-identical; degenerate input now reports "not computable" rather than publishing a plausible-looking noise floor.
  • 9 the CuPy guard caught only ImportError; a driverless host raises RuntimeError, so the intended CPU-only path aborted.
  • 11 ROI threshold channel keys were hand-mapped with a silent fallback — a drift made DAPI grading 2.67x stricter (intensity_warn is 0.40, not 0.35) with nothing in the output revealing it. Replaced with a case-insensitive resolve that raises on a genuinely missing channel; all resolved values verified unchanged. The same pattern in _pick_critical was also live.
  • 8 params.stain_names is dropped. It was parsed and discarded, and the metric keys it would have to rewrite are the contract the threshold YAML and report read.

Also addressed: GPU handling now gates on use_gpu and caps devices via --max-gpus, with process_gpu_qc defined in conf/base.config and routed in the aws profile (it previously existed unrouted, and on the old branch inside an unclosed block, so it had never applied); publishDir no longer double-nests; QC parameters moved into the schema's QC group with a minimum on image_qc_gpus; resources come from dev's own labels rather than ad-hoc numbers; and the modules read no params.*.

Nothing ran the QC Python in CI

That is why several of the above survived. This adds .github/workflows/python-tests.yml and bin/tests/ (upstream's own suite, adapted only for layout). It builds a runtime environment, because both scripts import matplotlib/tifffile/scanpy at module scope and cannot otherwise be collected.

It also repairs the existing suite: tests/test_xenium_patch/test_stitch_transcripts.py imported the script from an upstream-only modules/local/.../resources/usr/bin path that does not exist here, so it failed collection and took the whole pytest run down (17 passed, 64 errors before). Result now: 87 passed, 1 skipped.

Validation

  • Real production Xenium bundle (126K cells, 7.8 GB): pipeline completes, image QC status: ok with its full metrics set, both reports render with no failure banner (58 MB / 19 figures, 3.8 MB / 11 figures)
  • Full nf-test pipeline suite green under the CI-pinned Nextflow 26.04.6, snapshots stable across an update and a separate verify pass
  • make check clean (ruff, mypy over 27 files, pre-commit); nf-core pipelines lint schema checks clean

Four pre-existing mypy failures unrelated to QC are fixed so make check passes (Pillow 12 dropped Image.NEAREST; a rebound tuple annotation; a headerless-CSV None; two str arguments declared as Path). mypy.ini excludes only the four vendored QC scripts, since annotating them locally would fork them from upstream.

Known issues, not introduced here

  • SPOQC_ANALYSIS_OVERVIEW flakes. It failed in three of seven runs with ValueError: Too many bins for data range from spoqc/additional_analysis/cluster_analysis.py, and reproduces with the QC subworkflow call removed. It histograms clustering output derived from the spatialdata parquet writers, which tests/.nftignore already documents as not run-to-run reproducible, so the 5-bin histogram occasionally sees a degenerate range. @heylf — this will flake in CI regardless of this PR.
  • Containers are on a personal namespace. quay.io/dongzehe/image_qc:1.0.0 and transcript_qc:1.1.0, both public. They need migrating to the nf-core org before release. The transcript tag is 1.1.0 because its environment genuinely changed and 1.0.0 was already published from a different one — and because pyarrow=21.0.0 made the declared environment unbuildable (it conflicts with scanpy 1.11.4), now pinned to 20.0.0.

Reviewers

@nnie-altos @nmalwinka @ChristelKrueger for the QC content, @heylf for nf-core conventions and the spoQC interaction.

an-altosian and others added 4 commits August 21, 2026 16:58
@an-altosian @nmalwinka
@nnie-altos @ChristelKrueger
feat(qc): add image QC and transcript QC from nf-xenium-processing de…
c775ab5 
…v HEAD

Ports the QC layer from the internal nf-xenium-processing repo at dev HEAD
5e35cae onto nf-core/spatialaxe dev, alongside the existing spoQC subworkflow.

Faithfulness to the source was the priority, because the previous attempt
shipped a stale, partial snapshot: its transcript QC was 695 lines against
upstream's 1344, its report notebook 333 against 1861, and transcript_stream.py
and the threshold config were missing entirely. Reviewers spent their time
re-deriving issues that current upstream had already fixed.

Provenance, verified rather than asserted:

- bin/snr_metrics.py, bin/xenium_image_qc_report.qmd,
  bin/roi_image_qc_thresholds.yaml, bin/transcript_stream.py: byte-identical to
  upstream 5e35cae.
- bin/image_qc.py: exactly three diff hunks vs upstream, none touching logic —
  a typing import, removal of the xenium_helpers import region, and the inlined
  helper block replacing it. The seven inlined symbols are byte-identical to
  upstream's xenium_helpers.utils (AST-compared).
- bin/transcript_qc_processing.py, bin/transcript_qc.qmd,
  bin/transcript_qc_thresholds.yaml: upstream plus a mechanical
  molecule -> transcript rename. The rename is recorded as an ordered,
  replayable rule set; replaying it on pristine upstream reproduces the
  notebook and threshold YAML byte-for-byte, so re-syncing stays a mechanical
  step rather than a merge.

xenium_helpers is dropped completely: the needed helpers are inlined into each
script verbatim, with no shared shim module.

This picks up four upstream fixes the stale port missed: the under-scaled noise
threshold (now a separate scaled_noise_threshold function), unbounded host
memory (now a streaming reader via transcript_stream.py), figure names that did
not match their contents, and uncomputed unassigned% / per-FoV QV.

Pipeline integration:

- nf-core quarto/notebook renders both reports. The older quartonotebook module
  is deprecated upstream and now hard-asserts, so it is not used.
- The versions topic collector now keeps only (process, tool, version) tuples:
  quarto/notebook also publishes a versions.yml path into that topic, which
  broke the destructuring.
- Resources come from dev's own labels rather than ad-hoc numbers. Image QC
  carries process_gpu_qc (GPU request only) plus process_xl, which matches what
  a 230K-cell production bundle actually needed; transcript QC uses
  process_high, matching upstream's allocation.
- process_gpu_qc is defined in conf/base.config and routed in the aws profile;
  it previously existed unrouted and, on the old branch, inside an unclosed
  block, so it had never actually applied.
- The QC processes read no params.* — every knob arrives as ext.* from
  conf/modules.config. Four of those keys must stay explicit because the module
  tests them negated or interpolates them unconditionally.
- publishDir no longer double-nests: analysis and report both land in
  <mode>/qc/<step>/.
- 18 QC parameters are declared under the schema's QC options group, with a
  minimum on image_qc_gpus so a negative device count is rejected at launch;
  run_qc moved out of Segmentation options.

Also fixes four pre-existing mypy failures unrelated to QC (Pillow 12 dropped
Image.NEAREST; a rebound tuple annotation; a headerless-CSV None; two str
arguments declared as Path), so `make check` passes. mypy.ini excludes only the
four vendored QC scripts, since annotating them locally would fork them from
upstream.

Validated: full -stub run in qc mode executes all four QC processes and
publishes reports and metrics; make check clean (ruff, mypy over 27 files,
pre-commit); nf-core schema lint 468 passed / 0 failed.

Co-authored-by: Malwina Prater <mprater@altoslabs.com>
Co-authored-by: Nell Nie <nnie@altoslabs.com>
Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
@an-altosian @nmalwinka
@nnie-altos @ChristelKrueger
fix(qc): address reviewer findings and run the QC Python tests in CI
edbec26 
Reviewer items that needed real fixes (numbering from the PR review):

- 6: --non-gene-prefix and --stain-names had no `nargs`, so the multi-value
  form that nextflow_schema.json documents and the module emits (it splits on
  ';' into separate tokens) exited with argparse error 2. Added nargs="+".
  Verified by driving parse_args: two values previously raised SystemExit(2)
  and now parse to a list.
- 9: the CuPy import guard caught only ImportError. The container ships
  cupy-cuda12x with CUDA runtime wheels but no driver, which raises
  RuntimeError/CUDARuntimeError at import — neither an ImportError — so the
  intended CPU-only default path aborted instead of falling back. Broadened
  the guard. Verified with a stub cupy raising RuntimeError at import.
- 7: estimate_min_mols_per_cell raises from np.quantile on an empty `> mode`
  slice, reachable with zero/one cell or zero retained genes (numpy raises
  IndexError, not ValueError). Guarded at the two call sites rather than inside
  the vendored helper, so the helper stays byte-identical to upstream. A
  degenerate input now yields "not computable" (None -> null in the metrics
  JSON, cutoff line omitted) instead of an exception; returning the helper's
  min_value would have published a real-looking noise floor for empty data.
  Normal inputs are unchanged (verified: 48 before and after).
- 5: --non-gene-prefix was parsed and then ignored — the selection hard-coded
  startswith("NegControl"). Threaded the prefixes through
  scaled_noise_threshold and changed the script default from "NegControlProbe"
  to "NegControl" so the default reproduces the previous hard-coded behaviour
  exactly and matches the pipeline default; wiring it without that change would
  have moved the threshold. Verified: the parameter now changes the threshold
  across prefix sets, and the default is identical to the old literal.
- 11: the ROI threshold channel keys were bridged by a hand-written
  lowercase-to-mixed-case dict, and a drift between it and the YAML silently
  fell back to a 15% WARN fraction where the YAML says 40% — 2.67x stricter
  DAPI grading with nothing in the output revealing it. Replaced with a
  case-insensitive resolve that raises when a populated config is missing an
  expected channel. All resolved values verified unchanged for all three
  channels, with and without YAML, and for every XOA-version branch. The same
  pattern in _pick_critical was fixed too; without it every intensity floor
  silently reverted to the defaults, discarding the XOA-4 values.

Items 8 and 10 needed no code change and are recorded for the reviewers:
10 (the report instructed filtering at QV<=20 while nothing filtered) is gone
in current upstream, and the notebook now states explicitly that the threshold
uses only negative-control features — so the claim matches the code. 8
(--stain-names parsed then discarded) is confirmed still dead: every figure
title and metric key is hard-coded, and the metric keys are the contract the
threshold YAML and the report read, so making stain names rewrite them would
break that contract. Left for a deliberate decision.

CI: nothing ran the QC Python code, which is why these defects survived. Added
.github/workflows/python-tests.yml. It builds a runtime environment rather than
the linting one, because both scripts import matplotlib/tifffile/scanpy at
module scope and cannot even be collected otherwise. The two QC module
environments cannot be co-installed (image QC pins zarr 2.x while scanpy pulls
zarr 3.x), so bin/tests/environment.yml declares one coherent set.

Also repairs the Python suite itself: tests/test_xenium_patch/test_stitch_-
transcripts.py imported the script from an upstream-only
modules/local/.../resources/usr/bin path that does not exist here, so it failed
collection and took the whole pytest run down with it (17 passed, 64 errors
before this). It now loads bin/stitch_transcripts.py, raises a clear error if
that ever moves, and skips cleanly where shapely/sopa are absent instead of
aborting collection.

Verified: 87 passed / 1 skipped in the CI-equivalent environment; make check
clean; the vendored helper blocks re-checked byte-identical to upstream
(7/7 and 11/11 symbols) so re-syncs stay mechanical.

Co-authored-by: Malwina Prater <mprater@altoslabs.com>
Co-authored-by: Nell Nie <nnie@altoslabs.com>
Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
@an-altosian @nmalwinka
@nnie-altos @ChristelKrueger
fix(qc): make the QC scripts executable and satisfy the quarto versio…
ba2398e 
…ns contract

Three defects that only a real (non-stub) run could surface, since -stub never
executes the analysis scripts nor renders the notebooks:

- The four ported bin/ scripts were committed mode 644 while every other script
  in bin/ is 755. Nextflow runs bin/ scripts directly, so image QC failed
  immediately with exit 126 "Permission denied". Recorded the executable bit in
  the index, not just the working tree.
- The nf-core quarto/notebook module requires the notebook to export a
  versions.csv (package,version) which it folds into the versions topic; the
  upstream notebooks do not, because upstream renders with its own quarto
  module. Both reports rendered and then the task failed. Added a small cell to
  each notebook that reads the versions at runtime via importlib.metadata,
  matching the pipeline's rule that versions are never hardcoded. This is a
  third documented divergence in the transcript notebook, so the recorded
  rename replay no longer reproduces it byte-for-byte and the cell has to be
  re-applied on re-sync.
- The versions collector I had hardened to ignore non-tuple topic entries was
  discarding the versions.yml that quarto/notebook builds from that very
  versions.csv — requiring an artifact and then binning it. It now handles both
  shapes: tuples are formatted directly, and file entries are read in as YAML
  content (softwareVersionsToYAML parses content, not paths).

Validated on a production Xenium bundle (126K cells, 7.8 GB): the pipeline
completes, image QC reports status ok with its full metrics set, and both
reports render with no failure banner — image_qc.html 58 MB / 19 figures,
transcript_qc.html 3.8 MB / 11 figures. The report section of the software
versions table now carries the notebook's own numpy, pandas and ipython
versions.

Co-authored-by: Malwina Prater <mprater@altoslabs.com>
Co-authored-by: Nell Nie <nnie@altoslabs.com>
Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
@an-altosian @nmalwinka
@nnie-altos @ChristelKrueger
feat(qc): drop the stain_names parameter, document QC, fix the contai…
a5241ec 
…ner env

- Drop `params.stain_names` (reviewer item 8). The script parses `--stain-names`
  and then discards it: every figure title and metric key is hard-coded, and
  those keys are the contract the threshold YAML and the report read, so making
  stain names rewrite them would break that contract. Shipping a knob with no
  effect is worse than not having it, so the parameter, its schema entry and the
  `ext`/arg wiring in both modules are removed. The vendored scripts keep their
  now-unreachable CLI option rather than diverging further from upstream; each
  module records why it is not passed.
- `modules/local/transcript_qc/environment.yml`: `pyarrow` 21.0.0 -> 20.0.0. The
  pinned set was unsolvable — pyarrow 21 conflicts with scanpy 1.11.4 on
  python 3.11.0 — so the declared environment could not build at all. Earlier
  runs passed only because they used the previously published image. pyarrow 20
  still provides libarrow-acero, which transcript_stream.py needs.
- Bump the transcript QC container to 1.1.0 and point both references at it. The
  environment genuinely changed (scanpy moved from pip to conda, numba and tqdm
  added, six pip packages dropped), so 1.0.0 no longer describes it;
  overwriting a published tag would make earlier runs irreproducible. Image QC
  stays at 1.0.0 — its environment is unchanged and does solve.
- CHANGELOG and docs/output.md: describe the QC subworkflow, every published
  output file, and the new parameters, which were previously undocumented.
- tests/.nftignore: ignore the QC report HTMLs and figure PDFs, which embed a
  render timestamp and a PDF CreationDate respectively. Names and existence are
  still asserted.
- Rebaseline all six pipeline snapshots: the QC layer adds published outputs by
  design, and spoqc.nf.test runs `mode = 'qc'` so its output set changes too.

Validated: full nf-test pipeline suite green under the CI-pinned Nextflow
26.04.6, with the snapshots stable across an update and a separate verify pass;
make check clean; 87 pytest tests pass.

Note for reviewers: SPOQC_ANALYSIS_OVERVIEW fails intermittently (three of
seven runs here, on unmodified dev as well as on this branch) with
`ValueError: Too many bins for data range` raised from
spoqc/additional_analysis/cluster_analysis.py. It histograms clustering output
derived from the spatialdata parquet writers, which tests/.nftignore already
documents as not run-to-run reproducible, so the 5-bin histogram occasionally
sees a degenerate range. It is unrelated to this PR — it reproduces with the QC
subworkflow call removed — but it will flake in CI.

Co-authored-by: Malwina Prater <mprater@altoslabs.com>
Co-authored-by: Nell Nie <nnie@altoslabs.com>
Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 4.0.3.
Please update your pipeline to the latest version.

For more documentation on how to update your pipeline, please see the Synchronisation documentation.

@github-actions

github-actions Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 1303d6c

+| ✅ 295 tests passed       |+
#| ❔  10 tests were ignored |#
#| ❔   1 tests had warnings |#
!| ❗   5 tests had warnings |!
Details

❗ Test warnings:

  • files_exist - File not found: conf/igenomes.config
  • files_exist - File not found: conf/igenomes_ignored.config
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • schema_lint - Schema 'description' should be 'A pipeline for spatialomics 10x Xenium In Situ data.'
    Found: 'A pipeline to process spatialomics data from 10x Xenium In Situ or 10x Atera.'

❔ Tests ignored:

  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/linting_comment.yml
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: assets/nf-core-spatialaxe_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-spatialaxe_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-spatialaxe_logo_dark.png
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore
  • template_strings - Ignoring Jinja template strings in file /home/runner/work/spatialaxe/spatialaxe/bin/transcript_qc.qmd
  • template_strings - Ignoring Jinja template strings in file /home/runner/work/spatialaxe/spatialaxe/bin/xenium_image_qc_report.qmd

❔ Tests fixed:

✅ Tests passed:

Run details

  • nf-core/tools version 4.0.3
  • Run at 2026-08-22 01:06:18

@an-altosian
fix(ci): exempt the QC notebooks from template_strings and fix the te…
01ee28a 
…st env

Two CI failures on this PR, both self-inflicted:

- nf-core lint template_strings flagged bin/xenium_image_qc_report.qmd: the QC
  report notebooks use doubled braces as Python f-string escapes (they emit
  pandoc callout-note divs), which the check reads as leftover Jinja. Added the
  documented per-file exemptions.
- The Python tests workflow could not create its environment. It still merged
  the two QC module environments, which cannot be co-installed — transcript QC's
  anndata 0.12.2 requires pyarrow <21 while image QC pins pyarrow 21.0.0. That
  is also the real constraint behind the unbuildable container environment
  fixed in the previous commit; anndata, not scanpy, is what caps pyarrow.
  bin/tests/environment.yml was already made self-contained for exactly this
  reason, so the workflow now uses it alone.

Also ignore .mypy_cache/, whose binary cache files make the merge_markers lint
check report false positives locally.

Verified: template_strings passes (366 tests, 0 failed) and
bin/tests/environment.yml solves standalone.
@github-actions

github-actions Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown

❌ nf-test failed with latest Nextflow version

Note

Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
Please check if the failure is expected with newer (edge-)releases of Nextflow or if it needs fixing.

  • ❌ docker | latest-everything | Shard 10/12

See the full run for details.

@an-altosian
ci: give nf-test shards more disk for the QC containers
1303d6c 
A shard failed with `docker: failed to register layer: write
/xeniumranger-xenium4.0/external/cellranger/lib/bin/cr_ana: no space left on
device` while pulling quay.io/nf-core/xeniumranger:4.0. The QC layer adds two
sizeable images (image QC ~4.6 GB, transcript QC ~3 GB), and a shard that also
pulls xeniumranger — which bundles cellranger — no longer fits in the 80 GB
volume. Raise it to 120 GB.

The same shard passed on latest-everything, whose runner had different residual
disk, which is why this looked intermittent rather than systematic.

Follow-up worth doing separately: the image QC container carries transitive
weight it does not need (botocore ~121 MB, sympy ~81 MB, statsmodels ~56 MB and
PySide6 ~43 MB, pulled in via the optional pysal/esda stack used only when
Moran's I is enabled, which is off by default). Trimming those would cut the
image without touching GPU support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@an-altosian