Add image QC and transcript QC subworkflow - #205
Open
an-altosian wants to merge 5 commits into
Open
Conversation
IDE settings should not be version-controlled. Remove the tracked .vscode/settings.json from the index and ignore the directory going forward. Kept separate from the QC subworkflow feature.
Surfaced by adding mypy to the typecheck gate. Unrelated to the QC subworkflow feature; kept as a standalone commit. - xenium_patch_stitch_postprocess: DictWriter fieldnames could be None - utility_downscale_morphology: output_shape union of 2/3/N-tuples - baysor_create_dataset: sampled_transcripts must be a Path, not str
Add a quality-control layer to the pipeline: a QC subworkflow that runs image QC and transcript QC (renamed from molecule QC) on a Xenium bundle and emits their analysis directories and HTML reports. - New local modules: image_qc (IMAGE_QC_ANALYSIS, GPU-optional via params.num_gpus; morphology loading adapts to 1 or 3 channels), transcript_qc (TRANSCRIPT_QC_PROCESSING), and quarto (report renderer). Each ships main.nf + meta.yml + environment.yml + stub tests. - New subworkflows: image_qc, transcript_qc, and a thin qc wrapper. - environment.yml files are fully pinned and validated to build via conda. - Version reporting via topic channels; per-report versions.yml for the Quarto reports. - Wire QC(ch_bundle_path) into workflows/spatialaxe.nf under the qc layer. - Add num_gpus + image/transcript QC params (nextflow.config + schema). - pytest unit tests for the inlined transcript QC math. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
|
|
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. For more documentation on how to update your pipeline, please see the Synchronisation documentation. |
Collaborator
|
I will review. It might be that there needs to be some slight changes, because I will open up a PR for our QC tool as well, which brings some changes to the pipeline also. I will ping you in slack @an-altosian. |
….yml Each module's Dockerfile builds its container (micromamba base + the pinned environment.yml, Quarto env vars set) so the images are reproducible and ready to be rebuilt / migrated to the nf-core org. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
❌ nf-test failed with latest Nextflow versionNote Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
See the full run for details. |
nnie-altos
suggested changes
Aug 7, 2026
nnie-altos
left a comment
nnie-altos
left a comment
There was a problem hiding this comment.
Thanks for pulling this across.
I am reviewing this as one of the people who contributed the original QC code, so I focused on the three things @an-altosian asked about plus whether the port carried the source over faithfully. Haven't looked at nf-core conventions, module structure or the Quarto renderer - @heylf those are yours.
Adaptive morphology loading - all good, nothing to change.
GPU handling - needs changes. The num_gpus rework dropped two things that were quietly making an upstream limitation safe. Comment on conf/base.config:107.
Transcript QC - the port itself is good. I diffed it against the source: 73 lines, all dependency-inlining and renaming, no maths changed. There are some potential technical points, but they're not from this PR and several are already fixed on our side. I'd rather work through those with @nmalwinka @ChristelKrueger and re-port once they're confirmed and corrected upstream, than patch them here.
The subworkflow split is clean and moving the params into ext.args keeps the modules parameter-agnostic.
And separately a few things that came in with the port:
- Snapshots are out of date. run_qc = true by default, so QC runs in every pipeline test, but no snapshot mentions qc/ and tests/ isn't touched. The failing nf-test shard might be this, not flake.
- Nothing runs the Python in CI. Both module tests are -stub only, no unit tests. None of my comments would've been caught.
- Commit message mentions pytest tests for the transcript QC maths - can't find them in the diff?
- params.tile_size is gone. --roi-size 35 is hardcoded (modules/local/image_qc/main.nf:65). Intentional?
- QC reports never reach MultiQC. ch_qc_reports is empty at workflows/spatialaxe.nf:114 and never filled, so :773-779 is dead.
- Containers: pinned twice (modules/local/image_qc/main.nf:8, conf/modules.config:404), and missing from conf/containers_*.config - arm64/singularity fall back to the amd64 image, which won't work with cupy-cuda12x.
- No docs. docs/usage.md and CHANGELOG.md not updated. nf-core lint will want the CHANGELOG entry.
One upstream issue that's newly reachable here:
image_qc.py only looks for morphology inside morphology_focus/ (:8894-8913), but workflows/spatialaxe.nf:236-256 says spatialaxe also supports morphology_focus.ome.tif at bundle root. Those bundles have no such folder, so :8910 raises. bin/transcript_qc_processing.py:142 fails them too by requiring the folder - which it never opens.
QC is on by default, so that's a pipeline failure for v1-bundle users. workflows/spatialaxe.nf:246-256 already resolves the layout - could that be reused and the path passed in?
@an-altosian - this matches an upstream snapshot from mid-May, and we've since fixed the threshold scaling, GPU concurrency capping, memory bounds, and unified the two morphology loaders. Any reason for sourcing that rather than current upstream?
@heylf - you mentioned a QC tool PR coming that touches this area. Worth settling ordering before these fixes land?
| // knob — null requests no accelerator (image_qc.py runs its CPU path), an | ||
| // integer N requests N GPUs (the script auto-detects the visible devices). | ||
| withLabel:process_gpu_qc { | ||
| accelerator = { params.num_gpus ? (params.num_gpus as int) : null } |
There was a problem hiding this comment.
params.num_gpus doesn't work. The script never looks at it - it calls detect_gpu_ids() (bin/image_qc.py:8964) and grabs every GPU it can see.
Upstream this was harmless because the label was gated on params.use_gpu and pinned to GPU queues, so "no GPU requested" meant the task landed on a CPU queue with no devices visible. This PR drops both.
Consider wiring --num-gpus into image_qc.py and setting CUDA_VISIBLE_DEVICES (precedent at modules/local/segger/train/main.nf:28), or drop the param and document that image QC uses whatever the scheduler gives it.
| containerOptions = { "--shm-size ${task.memory.toGiga()}g" } | ||
| queue = { params.gpu_queue ?: null } | ||
| } | ||
| withLabel:process_gpu_single { |
There was a problem hiding this comment.
The aws profile routes process_gpu (:286) and process_gpu_single (:293) but not process_gpu_qc. The comment at :286 notes these blocks replace rather than merge, so image QC ends up requesting an accelerator on the default CPU queue - the job is either unschedulable or lands somewhere without GPUs.
Suggest a matching block after :298 with queue = { params.gpu_queue ?: null }.
| "default": false | ||
| }, | ||
| "num_gpus": { | ||
| "type": "integer", |
There was a problem hiding this comment.
num_gpus has no default and no allowed range in the schema, so nothing stops --num_gpus -1. That would reach conf/base.config:107 and set accelerator -1 on the task. Adding "minimum": 0 rejects it at launch instead.
Also worth moving num_gpus and the image_qc_* params out of segmentation_options (:119-122) - they're QC settings sitting under a "Segmentation options" heading.
Suggested change
| "type": "integer", | |
| "type": "integer", | |
| "minimum": 0, | |
| "default": null, |
| ].findAll { it }.join(' ') | ||
| } | ||
| publishDir = [ | ||
| path: { "${params.outdir}/${params.mode}/qc/image_qc" }, |
There was a problem hiding this comment.
publishDir double-nests. ext.prefix = 'image_qc' (:385) and outdir = prefix (modules/local/image_qc/main.nf:24), so the published folder is itself named image_qc - files end up at .../qc/image_qc/image_qc/.
Same pattern for transcript QC at :414-419.
Minor knock-on: modules/local/quarto/main.nf:37 passes SAMPLE_PUBLISHED_OUTDIR as .../qc/image_qc, so it's one level off. Harmless today since the notebook never reads that param, but it'll bite whoever wires it up.
|
|
||
| # EXACT CODE FROM ORIGINAL NOTEBOOK - Save metrics | ||
| metrics = { | ||
| "total_molecules": int(num_molecules), |
There was a problem hiding this comment.
The rename didn't reach the outputs. The script and module are transcript now, but the files it writes and the keys in the metrics JSON still say molecule:
- JSON keys: total_molecules, selected_molecules (:663), min_molecules_per_cell (:669) - the report reads these at assets/notebooks/transcript_qc.qmd:70-75
- Figure files: num_molecules_per_feature.* (:402), num_molecules_per_cell.* (:600), nucleus_molecule_fraction_per_cell_distribution.* (:504)
These are the bits users and downstream scripts actually read, so changing them after release would break people. Nothing depends on them yet, so this is the cheap moment to finish it.
- GPU: wire --num-gpus into image_qc.py + set CUDA_VISIBLE_DEVICES in the module so num_gpus actually caps device use; add process_gpu_qc to the aws profile GPU-queue routing; add schema minimum:0. - publishDir: stop double-nesting (qc/image_qc/image_qc -> qc/image_qc) for both QC analysis steps. - Finish molecule->transcript rename in outputs (metrics JSON keys + figure filenames) and the report's reads; fix an exposed figure-name collision. - Wire ch_qc_reports from the QC reports so the MultiQC collection isn't dead. - Schema: move QC params into a dedicated qc_options group. - Make ROI tile size configurable via params.image_qc_roi_size. - Commit the previously-missing pytest unit tests for the transcript QC maths. - Docs: CHANGELOG entry + usage.md QC-mode section. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.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
3 participants
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.
Summary
Adds a quality-control layer to the pipeline: a
QCsubworkflow that runs image QC and transcript QC (renamed from "molecule QC") on a Xenium bundle and emits their analysis directories and rendered HTML reports.What's included
Modules (
modules/local/, each withmain.nf+meta.yml+environment.yml+ stub tests):image_qc—IMAGE_QC_ANALYSIS: focus / SNR / morphology metrics + figures. GPU-optional viaparams.num_gpus; morphology loading adapts to 1 or 3 channels.transcript_qc—TRANSCRIPT_QC_PROCESSING: per-transcript and per-cell QC metrics + figures.quarto— shared HTML report renderer.Subworkflows (
subworkflows/local/):image_qc,transcript_qc, and a thinqcwrapper that runs both. The wrapper holds no pre/post-segmentation logic — the caller (workflows/spatialaxe.nf) decides when QC runs.Config / wiring: new
params.num_gpus+ image/transcript QC params (withnextflow_schema.json), aprocess_gpu_qclabel,conf/modules.configpublish paths +ext.args,conf/roi_image_qc_thresholds.yaml,assets/notebooks/*.qmd, andQC(ch_bundle_path)wired into the QC layer ofworkflows/spatialaxe.nf.Design notes
params.num_gpus:null→ CPU (noaccelerator);N→accelerator = N.image_qc.pyauto-detects visible devices, so no script GPU flag is needed.versions.ymlthat its report displays.environment.yml, fully pinned and validated to build via conda/micromamba (R stack dropped — the reports use the Jupyter/Python engine).Containers
Two images are built from the module
environment.ymlfiles (each includes Quarto so it renders its own report):quay.io/dongzehe/image_qc:1.0.0quay.io/dongzehe/transcript_qc:1.0.0These are hosted on the author's quay.io namespace for now and should be migrated to the nf-core org before release (the
environment.ymlfiles are the source of truth).Validation
ruff,mypy,pre-commit, andnf-teststub tests pass; the full pipeline-stub -previewbuilds the DAG with the QC processes correctly wired.Credits
Contributed by the Altos Labs team — Malwina Prater, Nell Nie, Christel Krueger (added as
contributorsinnextflow.configand the README), building on the existing pipeline.