Conversation
…nnotation PAIRED_SAVANA and TUMORONLY_SAVANA only emitted the classified somatic VCF and the segmented copy number. Padfoot and ReConPlot also need the somatic BEDPE, the fitted purity/ploidy and the het-SNP allele counts, all of which the nf-core savana/classify, savana/cna and savana/to modules already produce. Add those three emits to both subworkflows and mix the paired and tumour-only streams into per-product channels in the main workflow. All are optional: absent without an SNP source, and cna/fitted_purity_ploidy absent when SAVANA finds no acceptable fit (No_fit_found_PARAMS.tsv). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…workflows Padfoot (functional annotation of somatic SVs + CNAs) runs once per available caller pair: Severus + top-ranked Wakhan integer CN VCF, and SAVANA classified somatic SVs + SAVANA segmented absolute CN. ReConPlot (rearrangement + copy-number figures) runs per CN/SV pair: ASCAT + Severus, Wakhan + Severus, and SAVANA alone, producing per-chromosome and genome-wide figures, an optional region focus panel and the harmonised CN/SV tables. SAVANA inputs come from PAIRED_SAVANA / TUMORONLY_SAVANA (plain VCF from the nf-core modules; Padfoot reads it via pysam, the ReConPlot wrapper uses the BEDPE). Samples without a copy-number fit are dropped before the map that builds the ReConPlot input, since the remainder join yields a shorter tuple for them. Neither tool is on Bioconda: the pinned source trees are fetched with WGET+UNTAR (or taken from --padfoot_dir / --reconplot_dir / --reconplot_pkg_dir) and run in digest-pinned public images (ghcr.io/tim-yu/padfoot-repeatmasker with RepeatMasker 4.2.4 + Dfam 4.0, ghcr.io/tim-yu/reconplot); recipes under containers/. Both are skipped in the test profiles (whole-genome inputs, multi-GB images) and covered by module-level nf-tests instead. Parameter checks for unsupported genome presets live in validateSvAnnotationParams(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d docs Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Nextflow stores each workflow body as a single string constant, which the
JVM caps at 65,535 characters. After merging dev (report, signatures and
Verdict blocks) the inline Padfoot and ReConPlot wiring pushed LRSOMATIC's
body to 66,936 characters and the pipeline failed to compile
("String too long"). PADFOOT_ANNOTATION and RECONPLOT_FIGURES now hold the
source staging, the caller-pair joins and the module calls, mirroring
PAIRED_SAVANA; the main workflow passes channel.empty() for skipped callers
and is back to 59,214 characters. No change to process inputs, published
paths or the conf/modules.config selectors.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… run time run_reconplot.R and R/ are pipeline glue (parsers for ASCAT, Wakhan, Severus and SAVANA output), so they now live in assets/reconplot, vendored from Tim-Yu/ReConPlot at the commit recorded in assets/reconplot/VERSION. RECONPLOT_FIGURES stages that directory directly; RECONPLOT_WGET and RECONPLOT_UNTAR and the reconplot_url / reconplot_dir parameters are removed. The ReConPlot R package itself is still fetched from its pinned upstream commit (reconplot_pkg_url / reconplot_pkg_dir) because -profile conda installs it at run time. versions.yml records the wrapper commit from VERSION. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Padfoot published severus_wakhan/ while ReConPlot published wakhan_severus/
for the same pair. Both now use <sv_caller>_<cn_caller>: ReConPlot writes
reconplot/{severus_ascat,severus_wakhan,savana}/ and the process aliases are
RECONPLOT_SEVERUS_ASCAT and RECONPLOT_SEVERUS_WAKHAN.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Padfoot and ReConPlot need whole-genome caller output, so the pipeline tests skip them. These -stub tests feed PADFOOT_ANNOTATION and RECONPLOT_FIGURES stand-in files named like the real caller output and check the pairing: Severus + the top-ranked Wakhan solution, ASCAT + Severus, SAVANA with and without allele counts, and that a sample missing one side of a pair (no integer-CN VCF, one HP bed, no SAVANA fit) is dropped rather than failing the run. Tagged small so they run on every pull request. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CHANGELOG entries take dev's linked one-to-three-sentence form (PR number to fill in once the pull request exists). README lists padfoot/ and reconplot/ in the output tree and the output paragraph. docs/usage.md says why Padfoot is pinned to the Tim-Yu fork (SAVANA input support not yet upstream). The padfoot_genome schema description moves its detail into help_text, the duplicated caller-pair name in docs/output.md is fixed, and the test-profile skip comments shrink to one line. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The whole-genome test has run through, so here is the update promised in the description. Setup: fresh environment on a UCL CS cluster node (Singularity 3.8.5, Nextflow 25.10.7, local executor, work dir on node-local disk which I think is nvme ssd), one paired 40x ONT tumour/normal from raw uBAMs (126 + 113 GB), pipeline defaults with every tool on only Outcome: The two new steps RSS labels were assigned fine: Padfoot 14.2 GB RSS in 1.5 min on Two things not about this PR that may interest you (I may raise them in the SAVANA follow-up issue): |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate findings remain in ReConPlot handling and Padfoot pairing.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (5)
What changed in this PR
Adds default-on Padfoot annotation and ReConPlot visualization for SV/CNA caller pairs, with SAVANA output wiring, containers, parsers, configuration, documentation, and tests.
Changes:
- Adds Padfoot and ReConPlot workflows and modules.
- Extends paired and tumour-only SAVANA outputs.
- Adds R parsers, containers, parameters, documentation, and nf-tests.
Review findings:
- Critical: fix uncompressed VCF handling and preserve ReConPlot pipeline failures.
- Moderate: handle missing SV mates, empty Padfoot inputs, and unconditional downloads.
- Nits: correct changelog placeholders, container documentation, output documentation, and usage admonitions.
| File | Description |
|---|---|
workflows/lrsomatic.nf |
Integrates the new downstream workflows. |
tests/.nftignore |
Ignores generated figures and logs. |
subworkflows/local/utils_nfcore_lrsomatic_pipeline/main.nf |
Validates new parameters. |
subworkflows/local/tumor_only/tumoronly_savana.nf |
Emits additional SAVANA products. |
subworkflows/local/tests/reconplot_figures.nf.test |
Tests ReConPlot pairing. |
subworkflows/local/tests/padfoot_annotation.nf.test |
Tests Padfoot pairing. |
subworkflows/local/reconplot_figures.nf |
Builds ReConPlot caller-pair inputs. |
subworkflows/local/paired/paired_savana.nf |
Emits additional paired SAVANA products. |
subworkflows/local/padfoot_annotation.nf |
Builds Padfoot inputs and pairing logic. |
README.md |
Documents new outputs and workflow stages. |
nextflow.config |
Adds runtime parameters. |
nextflow_schema.json |
Defines parameter schema entries. |
modules/local/reconplot/tests/nextflow.config |
Configures module tests. |
modules/local/reconplot/tests/main.nf.test.snap |
Stores ReConPlot snapshots. |
modules/local/reconplot/tests/main.nf.test |
Tests ReConPlot I/O. |
modules/local/reconplot/meta.yml |
Documents the module interface. |
modules/local/reconplot/main.nf |
Runs ReConPlot. |
modules/local/reconplot/environment.yml |
Defines R dependencies. |
modules/local/padfoot/tests/nextflow.config |
Configures module tests. |
modules/local/padfoot/tests/main.nf.test.snap |
Stores Padfoot snapshots. |
modules/local/padfoot/tests/main.nf.test |
Tests Padfoot I/O. |
modules/local/padfoot/meta.yml |
Documents the module interface. |
modules/local/padfoot/main.nf |
Runs Padfoot annotation. |
modules/local/padfoot/environment.yml |
Defines Padfoot dependencies. |
docs/usage.md |
Documents parameters and usage. |
docs/output.md |
Documents generated outputs. |
containers/reconplot/README.md |
Documents ReConPlot image builds. |
containers/reconplot/Dockerfile |
Builds the ReConPlot image. |
containers/padfoot/README.md |
Documents Padfoot image builds. |
containers/padfoot/Dockerfile |
Builds the Padfoot image. |
conf/test.config |
Disables unsupported full-genome steps in tests. |
conf/test_full.config |
Disables unsupported full-genome steps in full tests. |
conf/modules.config |
Configures arguments and publishing. |
CITATIONS.md |
Adds tool and dependency citations. |
CHANGELOG.md |
Records the new functionality. |
assets/reconplot/VERSION |
Pins the wrapper revision. |
assets/reconplot/run_reconplot.R |
Provides the plotting CLI. |
assets/reconplot/README.md |
Documents the vendored wrapper. |
assets/reconplot/R/utils_vcf.R |
Provides VCF helpers. |
assets/reconplot/R/utils_common.R |
Provides shared validation utilities. |
assets/reconplot/R/plotting.R |
Renders and saves figures. |
assets/reconplot/R/parsers/wakhan.R |
Parses Wakhan CN output. |
assets/reconplot/R/parsers/severus.R |
Parses Severus SV output. |
assets/reconplot/R/parsers/savana.R |
Parses SAVANA output. |
assets/reconplot/R/parsers/registry.R |
Registers parsers. |
assets/reconplot/R/parsers/lrsomatic.R |
Provides the lrsomatic parser shortcut. |
assets/reconplot/R/parsers/generic.R |
Provides generic table parsing. |
assets/reconplot/R/parsers/ascat.R |
Parses ASCAT CN output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Clarify usage of `--vep_revel` and `--vep_eve` options and licensing information for `vep_plugins`. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
ljwharbers
left a comment
ljwharbers
left a comment
There was a problem hiding this comment.
Thanks for this. The pairing logic is carefully done. I checked the wiring against dev @ 0cf00bf:
- SEVERUS, ASCAT and WAKHAN use the same meta subMap for paired and tumour-only samples, and SAVANA only joins with itself, so the joins hold.
- A sample missing one side of a pair is dropped rather than failing the run.
pipefailis set, so theRscript | teepipes fail when Rscript fails.- Empty SV sets pass through
validate_sv. - Moving the wiring into subworkflows is the right fix for the JVM's 64 KB method-size limit.
CI is green. Before merge, a few things need attention.
Should fix before merge
-
Every default run now needs internet at start-up.
PADFOOT_WGETandRECONPLOT_PKG_WGETdownload about 120 MB from GitHub at run start whenever the steps are enabled, and they are enabled by default. This happens even when no caller pair forms.nf-core pipelines downloaddoes not stage these tarballs, so on compute nodes without internet an upgrade breaks runs for users who never asked for these tools.- The ReConPlot package is already built into the container, so the download is only needed under
-profile conda. Please guard it. - Padfoot already runs in a custom image. Could it be built into that image at the pinned commit? That removes the WGET, and
--padfoot_dirstays for conda and development. It also means runs no longer depend on a personal fork being reachable at run time. - Related question for the maintainers: should
skip_padfoot/skip_reconplotdefault totrueuntil real-data fixtures exist?
- The ReConPlot package is already built into the container, so the download is only needed under
-
docs/output.mdsays ASCAT does not run on tumour-only samples, but it does. Ondev, ASCAT runs on tumour-only samples because ClairS-TO Verdict uses its output (ascat_chmixes inbranched_minimap.tumor_only). Soreconplot/severus_ascat/is produced for tumour-only samples too. That path is not covered by any test: the stub test only gives ASCAT output to pairedsample1. Please fix the sentence and add a tumour-only ASCAT + Severus sample toreconplot_figures.nf.test.
Strongly suggested
-
The stub tests count tasks but never check which files were staged. The comments say solution_1's integer VCF and HP BEDs "must be picked", but the assertions only count tasks per sample. A regression that stages solution_2, or the subclonal BEDs, would still pass. One option: have the stubs write their staged inputs, e.g.
ls cn_input sv_input > ${prefix}/staged.txt, and assert on that file. Same forPADFOOT. -
A crash in either step fails the whole run. Both steps inherit
errorStrategy 'finish', so a failure in an optional figure or annotation step at the end of a ~12 h whole-genome run marks the entire run as failed. Two known ways to trigger it:sanitize_regionsstops when--reconplot_regionsnames a contig that has no CN.run_reconplot.Rexits 1 when it writes no files.
Worth a deliberate decision. We have been bitten by blanket
'ignore'hiding failures in CI before, so an explicit warn-and-continue may be the better option. -
The Padfoot genome gate is written twice (see the inline comment).
Nits
containers/reconplot/Dockerfile: a comment still refers to the removedparams.reconplot_url.run_reconplot.R: the help for--baf-tracksays "SAVANA only", but the ASCAT parser supports it and the pipeline passes it for ASCAT.nextflow.config: the comment says "containers ship full Dfam 4.0", but the image has only the root and curated-consensus partitions. The container README is accurate.- CHM13 is a reference we run routinely, and Padfoot skips it with only a warning. A short note in
docs/usage.mdon supplying--padfoot_gff/--padfoot_rmfor CHM13 would help. - Out of scope, but thanks for the numbers:
SAVANA_RUNpeaked at 149 GB againstprocess_high's 72 GB, andSEVERUSat 49.6 GB againstprocess_medium's 36 GB. Worth a separate issue.
| padfoot_src = channel.value([[id: 'padfoot'], file(params.padfoot_dir, type: 'dir', checkIfExists: true)]) | ||
| } | ||
| else { | ||
| PADFOOT_WGET( channel.value([[id: 'padfoot'], params.padfoot_url]) ) |
There was a problem hiding this comment.
This runs on every default run, even when no caller pair forms, and needs GitHub access at start-up. nf-core pipelines download won't stage it. Could Padfoot be built into the (already custom) image at this commit, with --padfoot_dir kept for conda and development?
| reconplot_pkg = channel.value([[id: 'reconplot_pkg'], file(params.reconplot_pkg_dir, type: 'dir', checkIfExists: true)]) | ||
| } | ||
| else { | ||
| RECONPLOT_PKG_WGET( channel.value([[id: 'reconplot_pkg'], params.reconplot_pkg_url]) ) |
There was a problem hiding this comment.
The container already has ReConPlot installed, so this download is only needed under conda. Suggest guarding it on the conda profile or on !workflow.containerEngine and passing [] otherwise. The module's library(ReConPlot) check already copes with the package being absent.
| └── multiqc | ||
| ``` | ||
|
|
||
| The `padfoot` and `reconplot` directories are only present when the corresponding step is enabled (`--skip_padfoot`, `--skip_reconplot`); SAVANA's own output lives under `variants/savana`. Within them, each caller-pair subdirectory requires both of its callers to have produced output for that sample: `severus_wakhan` needs `--skip_wakhan false`, `severus_ascat` needs `--skip_ascat false` and a matched normal (ASCAT is not run for tumour-only samples), and the `savana` subdirectories additionally need SAVANA copy number, which is only produced when an SNP source is available (the phased germline VCF for paired samples, or the bundled 1000G panel for tumour-only samples) and SAVANA finds an acceptable purity/ploidy fit. |
There was a problem hiding this comment.
On dev, ASCAT does run on tumour-only samples (Verdict uses its output), so severus_ascat will appear for them too. Please update this sentence and cover that case in the stub test.
| // | ||
| def padfoot_genome = params.padfoot_genome ?: | ||
| (params.genome == 'GRCh38' ? 'hg38' : params.genome == 'CHM13' ? 'chm13' : null) | ||
| def padfoot_annot_ok = padfoot_genome && ((padfoot_genome in ['hg38', 'mm10']) || (params.padfoot_gff && params.padfoot_rm)) |
There was a problem hiding this comment.
The same logic lives in validateSvAnnotationParams(), with a comment saying the two must mirror each other. Could it be one helper in utils_nfcore_lrsomatic_pipeline that both places call, so they can't drift apart?
Summary
Adds two downstream steps for the SV and copy-number callers already in the pipeline, as discussed in #192:
<sample>/padfoot/{severus_wakhan,savana}/.<sample>/reconplot/{severus_ascat,severus_wakhan,savana}/.Both run by default and are switched off with
--skip_padfoot/--skip_reconplot.PAIRED_SAVANAandTUMORONLY_SAVANAgain three emits (somatic BEDPE, fitted purity/ploidy, het-SNP allele counts) that the two steps consume; no other change to the SAVANA stack from #189.Design notes
KolmogorovLab/Padfootthat adds SAVANA input (I will propose it upstream;--padfoot_dirtakes a local checkout). The ReConPlot wrapper (run_reconplot.R+ parsers for ASCAT, Wakhan, Severus and SAVANA) ships inassets/reconplot/; the ReConPlot R package is fetched from a pinned upstream commit because-profile condainstalls it at run time.ghcr.io/tim-yu/padfoot-repeatmasker(Padfoot deps + RepeatMasker 4.2.4 + Dfam 4.0) andghcr.io/tim-yu/reconplot(R + ReConPlot), both digest-pinned in the modulecontainerdirectives sonf-core pipelines downloadstages them. Recipes undercontainers/. Under conda, RepeatMasker uses the curated Dfam subset bundled with bioconda.<sv_caller>_<cn_caller>for both tools. SAVANA's own output stays where Add savana module #189 put it (variants/savana).Testing
nextflow lint: no new warnings (15 ondev, 15 here).pre-commit/prettier clean.nf-core pipelines lint: 0 failed.tests/default.nf.testpasses under Singularity with the snapshot unchanged (Padfoot and ReConPlot need whole-genome caller output, so-profile testandtest_fullskip them).-stubnf-tests for both subworkflows (tagsmall, so they run on every PR) check that each caller pair is formed for the right samples and that samples lacking one side are dropped. Module-level nf-tests forPADFOOTandRECONPLOT(stub).dev@ 0cf00bf, 2026-09-23):nextflow lintidentical todev;nf-test --tag small --profile +singularity39/39; the two modules driven through their real subworkflows on whole-genome Severus/Wakhan/ASCAT/SAVANA output of a 40x ONT pair reproduce the earlier run byte-for-byte (tables, gene lists, figure counts); chr22 paired + tumour-only end-to-end 75/75 tasks. A whole-genome paired run from raw uBAMs with every tool on (Padfoot, ReConPlot, SAVANA, Wakhan, ASCAT, VEP, modkit, report) is in progress. Hopufully finishing soon and will update.minimap2-> Clair3/ClairS/ClairS-TO -> LongPhase -> Severus + SAVANA -> Padfoot -> ReConPlot), twice on this branch. On the merged tree SAVANA finds no acceptable purity/ploidy fit for either chr22 sample (it did for the tumour-only sample on the pre-merge ClairS-TO 0.4.2 calls), so these runs prove the skip path: the joins drop the samples and the pipeline completes. The positive path is proven on whole-genome caller output of a 40x ONT pair (Severus, Wakhan, ASCAT, SAVANA): Padfoot with RepeatMasker on both pairs, ReConPlot on all three pairs including the focus panel with gene labels and BAF track, using the vendored wrapper.Known limitations / follow-ups
IntGenomicsLab/test-datasetsif you want them.-stub-runcurrently fails before reaching these steps (theCRAMINOstubs lack*.arrow, the nf-corelongphase/phasestub runstabixon a file it never creates, and theWAKHANandSEVERUSstubs create fewer or differently named files than they declare). Out of scope here; happy to send a separate PR.PADFOOT_ANNOTATIONandRECONPLOT_FIGURESfetch their sources (90 MB + 30 MB) at run start even when no caller pair forms for any sample; harmless, could be guarded on non-empty input.--chromosomesfrom the contigs list, and a few unexposed CNA knobs (which was implemented in my previous attempts of local savana module).PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).For sure generated with Claude Code and then audited by Boyu.