Skip to content

Add image QC and transcript QC subworkflow - #205

Open
an-altosian wants to merge 15 commits into
nf-core:devfrom
an-altosian:feat/qc-subworkflow-port
Open

Add image QC and transcript QC subworkflow#205
an-altosian wants to merge 15 commits into
nf-core:devfrom
an-altosian:feat/qc-subworkflow-port

Conversation

@an-altosian

Copy link
Copy Markdown
Collaborator

Summary

Adds 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 rendered HTML reports.

What's included

Modules (modules/local/, each with main.nf + meta.yml + environment.yml + stub tests):

  • image_qcIMAGE_QC_ANALYSIS: focus / SNR / morphology metrics + figures. GPU-optional via params.num_gpus; morphology loading adapts to 1 or 3 channels.
  • transcript_qcTRANSCRIPT_QC_PROCESSING: per-transcript and per-cell QC metrics + figures.
  • quarto — shared HTML report renderer.

Subworkflows (subworkflows/local/): image_qc, transcript_qc, and a thin qc wrapper 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 (with nextflow_schema.json), a process_gpu_qc label, conf/modules.config publish paths + ext.args, conf/roi_image_qc_thresholds.yaml, assets/notebooks/*.qmd, and QC(ch_bundle_path) wired into the QC layer of workflows/spatialaxe.nf.

Design notes

  • GPU-optional via params.num_gpus: null → CPU (no accelerator); Naccelerator = N. image_qc.py auto-detects visible devices, so no script GPU flag is needed.
  • Version reporting via topic channels (collected pipeline-wide). Each analysis script also writes a versions.yml that its report displays.
  • Environments are per-module 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.yml files (each includes Quarto so it renders its own report):

  • quay.io/dongzehe/image_qc:1.0.0
  • quay.io/dongzehe/transcript_qc:1.0.0

These are hosted on the author's quay.io namespace for now and should be migrated to the nf-core org before release (the environment.yml files are the source of truth).

Validation

  • ruff, mypy, pre-commit, and nf-test stub tests pass; the full pipeline -stub -preview builds the DAG with the QC processes correctly wired.
  • pytest unit tests for the inlined transcript QC math.
  • Real-data tested end-to-end through the actual modules:
    • Transcript QC on the nf-core test bundle (metrics, figures, report).
    • Image QC on a real multi-channel Xenium v2 bundle with SNR enabled (per-ROI Otsu SNR, morphology overview, 4-channel loading) and on a single-channel bundle (adaptive 1-channel path).

Credits

Contributed by the Altos Labs team — Malwina Prater, Nell Nie, Christel Krueger (added as contributors in nextflow.config and the README), building on the existing pipeline.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

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

Posted for pipeline commit ff19451

+| ✅ 299 tests passed       |+
#| ❔  10 tests were ignored |#
!| ❗   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/assets/notebooks/transcript_qc.qmd
  • template_strings - Ignoring Jinja template strings in file /home/runner/work/spatialaxe/spatialaxe/assets/notebooks/xenium_image_qc_report.qmd

✅ Tests passed:

Run details

  • nf-core/tools version 4.0.3
  • Run at 2026-08-08 14:32:53

@github-actions

github-actions Bot commented Aug 6, 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.

@heylf

heylf commented Aug 7, 2026

Copy link
Copy Markdown
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.

@heylf heylf self-assigned this Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

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 7/12

See the full run for details.

@nnie-altos nnie-altos left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread conf/base.config Outdated
// 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 }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread nextflow.config
containerOptions = { "--shm-size ${task.memory.toGiga()}g" }
queue = { params.gpu_queue ?: null }
}
withLabel:process_gpu_single {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }.

Comment thread nextflow_schema.json Outdated
"default": false
},
"num_gpus": {
"type": "integer",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Comment thread conf/modules.config Outdated
].findAll { it }.join(' ')
}
publishDir = [
path: { "${params.outdir}/${params.mode}/qc/image_qc" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread bin/transcript_qc_processing.py Outdated

# EXACT CODE FROM ORIGINAL NOTEBOOK - Save metrics
metrics = {
"total_molecules": int(num_molecules),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

an-altosian added a commit to an-altosian/spatialaxe that referenced this pull request Aug 7, 2026
- 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>
an-altosian and others added 6 commits August 7, 2026 18:41
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>
….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>
- 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>
… dev d71e0cd)

The image QC module was ported from a stale mid-May upstream snapshot. Re-port
it verbatim from current upstream, which since fixed threshold scaling, GPU
concurrency capping (--max-gpus), host-memory bounds, tile streaming, and unified
the morphology loaders.

- bin/image_qc.py, bin/snr_metrics.py, assets/notebooks/xenium_image_qc_report.qmd:
  replaced verbatim with current upstream (byte-identical).
- modules/local/image_qc/main.nf: re-ported faithfully; thin nf-core adaptations
  (topic-channel versions, conda + quay container, when: guard).
- environment.yml: re-derived (cupy-cuda12x==14.0.1) + folded Quarto report stack.
- params reconciled to upstream: add tile_size, image_qc_gpus, image_qc_stream_tiles,
  image_qc_figures, image_qc_figure_source_tables; drop num_gpus, image_qc_roi_size.
  process_gpu_qc accelerator gated on use_gpu + capped by image_qc_gpus.
- mypy.ini: exclude the vendored QC analysis scripts from type-checking so they stay
  byte-identical to upstream and re-syncable.

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
an-altosian force-pushed the feat/qc-subworkflow-port branch from 7a3cce1 to f1dd859 Compare August 7, 2026 21:17
@an-altosian

Copy link
Copy Markdown
Collaborator Author

Thank you @nnie-altos , this is completely on me. I did not pull the latest commits when I was working on the PR. I have re-made the PR using our latest internal commits. Please check!

an-altosian and others added 9 commits August 7, 2026 21:42
CI on PR nf-core#205 surfaced two lint failures and container failures:

- nf-core lint: the three Altos contributor entries in the manifest had a
  trailing comma after their last field. Groovy tolerates it but nf-core's
  linter converts the manifest to JSON and choked (JSONDecodeError). This also
  cascaded into spurious files_exist igenomes failures because the config-aware
  exemption depends on a parseable manifest. Removing the trailing commas clears
  both.
- nf-test: QC processes died with exit status 1 (`Command 'ps' required by
  nextflow to collect task metrics cannot be found`). The micromamba-based QC
  containers lacked procps. Added conda-forge::procps-ng to all three QC
  environment.yml files (source of truth for docker/singularity/conda).

Co-authored-by: Malwina Prater <mprater@altoslabs.com>
Co-authored-by: Nell Nie <nnie@altoslabs.com>
Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
Adopt the official nf-core quartonotebook module (vanilla, no patch) instead of
the bespoke local quarto module:

- install modules/nf-core/quartonotebook (pinned in modules.json)
- rewire IMAGE_QC and TRANSCRIPT_QC subworkflows to its 4-input contract:
  parameters as a map (params.yml / --execute-params) and the analysis output
  directory (+ ROI YAML for image QC) staged via input_files; all per-sample
  channels derive from one upstream channel so emission order stays aligned
- pass the notebook as a plain path from the QC wrapper (meta2 tuple dropped)
- per-caller container overrides remain in conf/modules.config (the stock
  quartonotebook container lacks pandas, which both reports import); publishDir
  scoped to *.html so params.yml / notebook copies are not published
- emit report from REPORT.out.html; delete modules/local/quarto

Validated: -preview DAG, full -stub run, and a real (non-stub) qc-mode run on
the nf-core test bundle — both reports render with figures and no failure
banner.

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

Real-data validation (non-stub qc-mode runs) surfaced two failures that stub
tests cannot catch:

- image_qc.py failed at import: the verbatim upstream script depends on the
  xenium_helpers package, which this pipeline deliberately does not ship (QC
  scripts must be self-contained). Inlined the three segmentation-software
  label helpers (read_xenium_analysis_sw_version, read_xenium_major_version,
  resolve_segmentation_software, plus private helpers) verbatim from the same
  upstream commit (nf-xenium-processing dev d71e0cd); only annotations were
  modernised to X | None style. mypy.ini comment updated accordingly.
- generate_tissue_mask crashed with 'OSError: GL ES 2.0 library not found':
  napari-simpleitk-image-processing lazily imports napari -> vispy, which
  dlopens a GL ES library at import even in headless runs. Added
  conda-forge::libgles to the image_qc environment and set
  ES2_LIBRARY=/opt/conda/lib/libGLESv2.so.2 in the Dockerfile (vispy checks
  ES2_LIBRARY before ctypes.util.find_library, which cannot see /opt/conda/lib).

With both fixes the qc-mode run on the nf-core test bundle completes with
status ok, the full metrics suite, and a banner-free report.

Co-authored-by: Malwina Prater <mprater@altoslabs.com>
Co-authored-by: Nell Nie <nnie@altoslabs.com>
Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
Pillow 12 type stubs no longer declare the top-level Image.NEAREST alias, which
fails mypy. Image.Resampling.NEAREST is the supported form since Pillow 9.1 and
is available in the module's cellpose container.
Reprocessing a production Tower dataset (BII-002-715 R2_control, 126K cells)
exposed a silent failure: QUARTONOTEBOOK's process_low label (12 GB) OOM-killed
quarto's second pandoc pass (--embed-resources re-reads the entire rendered
HTML) on the ~60 MB image QC report. The child is SIGKILLed, quarto exits 1
with an empty stderr, and the complete HTML is left in the work dir — a
confusing signature. Confirmed by A/B: identical render succeeds unconstrained
and fails under --memory 12288m.

Give both REPORT processes explicit cpus/memory (42 GB, scaling with
task.attempt) in conf/modules.config. Config-only: the nf-core module stays
unpatched. With this fix the full qc-mode run on the production bundle
completes: 58 MB image QC report with 19 figures, no failure banner.

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

The two failing lint tests on PR nf-core#205 were template_strings: the QC Quarto
notebooks use doubled braces as Python f-string escapes (emitting pandoc
callout-note divs), which nf-core lint flags as leftover Jinja. Add the
documented per-file exclusions in .nf-core.yml. Verified locally with
nf-core/tools 4.0.3 (the CI-pinned version): 678 passed, 0 failed.

Also reorder the Altos manifest.contributors entries so a scalar field
(affiliation) is last: tools 4.0.3 converts the contributors block to JSON
with naive bracket replacements, and an entry ending in a list field
(contribution) gets its closing brackets corrupted, breaking RO-Crate metadata
generation (reproduced offline against the 4.0.3 transform). This was a
non-fatal log error, not the failing lint test.

Add .mypy_cache/ to .gitignore (local cache; its binary content trips
merge_markers when linting a dirty working tree).

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

The pipeline-level snapshots were baselined before the QC layer existed, so
every 25.04.0 shard failed with additions-only diffs (qc/ directories, reports,
versions.yml entries). Regenerated all six pipeline test snapshots under the
CI-pinned toolchain (Nextflow 25.04.0, nf-test --profile=+docker).

Local A/B/B reproducibility runs surfaced two flake classes, both fixed at the
narrowest possible scope while keeping name/existence assertions strict:

- content-unstable files (md5 changes every run): QC report HTMLs embed render
  dates and matplotlib figure PDFs embed CreationDate; MultiQC moved under
  <mode>/qc/ so its known-unstable outputs escaped the existing .nftignore
  patterns. Added QC-era paths to tests/.nftignore.
- set-unstable files: the pre/post-XeniumRanger MultiQC _report_data stats
  tables and _report_plots exports are sometimes omitted within a run. Ignored
  those directories in the coordinate test's stable_name list (the report html
  itself remains asserted) and content-ignored them wholesale in .nftignore.

Verified: coordinate test green 3 consecutive times, full suite green, under
the exact CI invocation.

ro-crate-metadata.json: regenerated; its embedded README copy now carries the
Altos contributor credits (enabled by the manifest.contributors parse fix).

Co-authored-by: Malwina Prater <mprater@altoslabs.com>
Co-authored-by: Nell Nie <nnie@altoslabs.com>
Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
The gene-panel fallback channel built `bundle + '/gene_panel.json'` with
file(checkIfExists: true) inside a .map closure that runs for every sample
even when the channel is never consumed (Nextflow evaluates map closures
regardless of downstream subscription, and the HTTP/file existence check
fires at file() construction on all Nextflow versions — verified identically
on 25.04.0 and 25.10.4). Any bundle without the optional gene_panel.json, or
a remote tarball URL input, therefore killed the workflow at init even with
relabelling disabled.

Guard the fallback construction with do_relabel: when relabelling is off,
ch_gene_panel keeps its channel.empty() initialisation, whose only consumer
already sits inside if (do_relabel). Behavior with relabelling enabled is
unchanged.

Verified: with a tarball-URL bundle and no test profile, the gene_panel init
error is gone (only the required-file checks remain); full nf-test suite green
with unchanged snapshots.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its better to move this to bin/ because it is directly linked to the .py script.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its better to move this to bin/ because it is directly linked to the .py script.

Comment thread conf/modules.config
Comment on lines +405 to +406
cpus = { 2 * task.attempt }
memory = { 42.GB * task.attempt }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesnt look like ery specific resource. Can you pick or define a label from/for base.config.

Comment thread conf/modules.config
Comment on lines +430 to +431
cpus = { 2 * task.attempt }
memory = { 42.GB * task.attempt }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesnt look like ery specific resource. Can you pick or define a label from/for base.config.

Comment thread conf/modules.config
// ---------------------------- image + transcript QC -----------------------

withName: '.*IMAGE_QC:ANALYSIS' {
ext.prefix = 'image_qc'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

tuple(
meta,
// parameters (NON_GENE_PREFIX is the key the module's arg builder reads)
["NON_GENE_PREFIX", params.neg_control_prefix],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all params.* in the main.nf needs to go into here

spatialaxe/main.nf

Lines 41 to 45 in b05a853

SPATIALAXE (
samplesheet,
params.alignment_csv,
params.baysor_config,
params.baysor_prior,

or the conf/modules.config

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, is this used somewhere? Or is this just a script you use internally for testing the tool? I would generally place it in bin/tests/ for now.

Comment thread nextflow.config
[
name: 'Malwina Prater',
contribution: ['contributor'],
affiliation: 'Altos Labs'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you have orcid please add

Comment thread nextflow.config
[
name: 'Nell Nie',
contribution: ['contributor'],
affiliation: 'Altos Labs'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you have orcid please add

Comment thread nextflow.config
[
name: 'Christel Krueger',
contribution: ['contributor'],
affiliation: 'Altos Labs'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you have orcid please add

@nmalwinka

nmalwinka commented Aug 21, 2026

Copy link
Copy Markdown

Following up on @nnie-altos's review, where she flagged the inherited transcript QC issues as ours
to confirm upstream before asking for changes here. I've now been through them. Here's the list,
trimmed to what's still open at cfff775, with the upstream status on each one so it's clear what a
re-sync buys you.

Items 1 to 11 below are @nnie-altos's findings, from her code read of the ported scripts. My part was confirming each one against current upstream, working out which already have a fix there, and measuring the size of the first one. The only item that's mine is the conf/base.config bracket at the end, which post-dates her review.

Three items from that list are already resolved: the GPU focus-map concurrency cap, the GPU OOM
fallback, and the unified morphology loader. The image QC re-port in f1dd859 picked all three up.

Summary: 4 of the 11 items below already have a fix upstream. A transcript QC re-sync closes them
with no new work.

# item upstream status
1 Noise threshold under-scaled fix exists upstream
2 Host memory unbounded fix exists upstream
3 Figure name / label inversion fix exists upstream
4 Unassigned% and per-FoV QV not computed fix exists upstream
5 neg_control_prefix has no effect not fixed anywhere
6 Documented multi-value form crashes not fixed anywhere
7 Crash on empty / all-zero cell counts not fixed anywhere
8 --stain-names parsed then discarded not fixed anywhere
9 CuPy import guard too narrow not fixed anywhere
10 QV filtering described but never applied not fixed anywhere
11 ROI threshold channel keys hand-mapped, silent fallback not fixed anywhere

Fix already exists upstream, so a re-sync picks these up

1. Noise threshold is under-scaled

Status: fixed upstream. Worth flagging that the upstream fix is @an-altosian's own commit, from
26 July. It just didn't travel, because the transcript QC script here matches an upstream snapshot from around
21-24 April, a month older than the mid-May snapshot the image QC came from.

bin/transcript_qc_processing.py:213-214 caps the non-gene frame at 1,000,000 rows, so the
NegControl per-probe counts fed to calculate_noise_bound are pre-scaled by 1e6 / n_nongene.
Line 367 corrects only for row-group subsampling, and on the default path num_row_groups is
None, so that ratio is exactly 1.0 and the line does nothing. The per-gene counts it's compared
against are scaled from the full frame (:379-381, compared at :416), so the threshold lands too
low by n_nongene / 1e6.

The bias is toward passing QC. I measured it across our own sections: it bites on roughly half
of them, and on the largest the threshold is more than 5x too low, which leaves the noise filter
doing essentially nothing. It propagates, because retained_genes is the gene set used to subset
the AnnData at :585, so min_transcripts_per_cell, min_genes_per_cell, retained_genes_count
and analyzed_genes all derive from it.

Upstream extracts the scaling into its own function, which is also the only way to test it: a test
of calculate_noise_bound alone can't see a bug that lives in the caller.

2. Host memory is unbounded on the default path

Status: fixed upstream. :178-188 reads the whole transcripts table eagerly. Six columns by a
few hundred million rows for a production slide, four landing as pandas object dtype, under
process_high (72 GB on attempt 1). --num-row-groups exists but is unreachable: the module can
pass it at main.nf:56-58, but subworkflows/local/qc/main.nf:52 only supplies NON_GENE_PREFIX
and no such param exists.

Upstream restructured this around a streaming reader and retired the flag, since streaming reads
the whole file at bounded memory and the flag only cost accuracy. Related and smaller, also fixed
upstream: read_random_parquet_row_groups (:76) takes no columns argument and is called bare at
:189, so even the row-group path reads every column rather than the six needed.

3. Figure names don't match their contents

Status: fixed upstream. cells_parquet["cell_size"] is plotted into
figures/genes_per_cell_distribution.{pdf,png} (:463, :468) with the matching
figures_source/genes_per_cell_distribution.csv (:479), so a cell-size KDE ships under a
genes-per-cell name. There's now also a separate num_genes_per_cell.*, which makes the two easy to
confuse.

Upstream splits these into three distinctly named outputs, one each for cell size, genes per cell
and molecules per cell.

4. Unassigned% and per-FoV quality aren't computed

Status: fixed upstream. assets/notebooks/transcript_qc.qmd:107 states "<5% unassigned" and
:313 claims spatial uniformity across fields of view, but neither appears in the metrics dict and
nothing reads x_location / y_location. cell_id is loaded and then unused.

Upstream computes a per-gene unassigned breakdown and per-FoV pct_unassigned and
pct_above_qv20, so the report's claims are backed by real numbers there.

Not fixed upstream either, so worth fixing in both repos

  1. params.neg_control_prefix has no effect. Parsed at :94-96, never read; :364 hardcodes
    startswith("NegControl"). So UnassignedCodeword_*, DeprecatedCodeword_*, BLANK_* and
    antisense probes sit outside the noise model, while transcript_qc.qmd:103 tells the reader
    deprecated probes are handled. The script default ("NegControlProbe") also disagrees with the
    pipeline default ('NegControl'), so wiring it up naively would move the threshold. Worth
    deciding deliberately which categories belong in the noise model, since the decoding-failure
    classes dominate the non-gene pool.

  2. The documented multi-value form crashes. modules/local/transcript_qc/main.nf:47-48 splits
    on ; and emits multiple bare tokens, but the option has no nargs='+', so
    --non-gene-prefix 'A' 'B' exits with argparse error 2. Harmless today only because the default
    has no semicolon, but nextflow_schema.json documents the ; syntax. Same for --stain-names
    at :52-53. Passing one quoted semicolon-separated string, as
    modules/local/image_qc/main.nf:52 does, avoids it.

  3. Crash on empty or all-zero cell counts. estimate_min_mols_per_cell raises from
    np.quantile on an empty > mode slice with one cell, zero cells, or zero retained genes,
    reachable via :589-593 and again at :632. On older numpy the slice yields nan and it
    becomes int(nan) instead. Note the ZeroDivisionError at :367 / :379-381 is a separate
    thing and is guarded upstream, but this one isn't fixed anywhere.

  4. --stain-names is parsed then discarded. stain_names_list is assigned at :13276,
    :13286, :13288 and only ever logged at :13282. Every figure title and metric key is a
    hardcoded "DAPI" / "Boundary" / "Interior". Either wire it up or drop params.stain_names;
    shipping a knob with no effect is worse than not having it.

  5. CuPy import guard is too narrow. It catches only ImportError, but
    modules/local/image_qc/environment.yml ships cupy-cuda12x plus CUDA runtime and nvrtc wheels
    with no driver, which is exactly when CuPy raises RuntimeError at import. That would crash the
    intended default CPU-only path. except Exception is the safe guard.

  6. QV filtering is described but never applied. transcript_qc.qmd:117 instructs filtering at
    QV 20 or below, but qv is only a plot axis and a cosmetic axvline. No filter is applied anywhere,
    upstream included. Either apply it or drop the instruction.

  7. ROI threshold channel keys are hand-mapped, and a mismatch fails silently. To be clear this
    one is not currently mis-grading anything, the mapping is correct today. It's the fragility I'd
    like to see removed. conf/roi_image_qc_thresholds.yaml uses two casings for the same three
    channels in the same file: DAPI / boundary / intRNA under channels: (:34, :59,
    :73), and dapi / boundary / intrna under snr: (:152, :158). bin/image_qc.py:7429
    bridges them by hand:

    _yaml_keys = {"dapi": "DAPI", "boundary": "boundary", "intrna": "intRNA"}
    ...
    _ch = _cpct.get(_yaml_keys.get(channel, channel)) or {}
    pct_warn_frac = float(_ch.get("intensity_warn", 0.15))

    If that dict and the YAML ever drift, .get() returns None, _ch becomes {}, and the WARN
    fraction silently falls back to 0.15. The YAML's DAPI value is 0.35, so DAPI grading would
    become 2.3x stricter with nothing in the output revealing it. The same lowercase-to-mixed-case
    pairing is repeated at :8926 (hardcoded .get("DAPI")), :13695, and in
    _INTENSITY_CRITICAL_DEFAULTS / _INTENSITY_DISPLAY_CAP. Normalising the YAML to one casing,
    or lowercasing keys on load, would remove the whole class of problem. Worth noting
    intensity_fail is no longer read at all (the code notes intensity stopped tracking quality
    post-XOA-4.0), so only the WARN tier is affected.

One new thing, post-dating that review

CI is red on cfff775 and reports Error base.config:203:1: Unexpected input: '<EOF>', which says
a block is unclosed but not which one. It's withLabel:process_gpu_qc, opening at
conf/base.config:183 and never closed, so withLabel:spoqc at :193 nests inside it and the file
ends one level deep. One } fixes it.

It came in with the dev merge rather than the original port (conf/base.config was balanced at
ff19451 and 8715e03). Note the docker | latest-everything shards pass while 26.04.6 fails
all 12, so the green ones are misleading. It also means the withLabel:process_gpu_qc settings have
never actually been applied in a run, so the GPU handling @nnie-altos raised is untested rather
than verified.

Happy to open issues for any of these that shouldn't gate this PR, and happy to help with the
transcript QC re-sync, which closes items 1 to 4.

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.

4 participants