Conversation
The module doesn't exist in nf-core/modules yet; this vendors it from a PR open against nf-core/modules (nf-core/modules#13021, with its own companion test-data PR nf-core/test-datasets#2289 for the bedmethyl fixture). modules.json records the source as YannVRB/modules.git@add-modkit-dmr rather than the canonical nf-core/modules.git, matching what `nf-core modules install --git-remote` produces for an unmerged module -- to be re-pointed at the canonical repo/branch/sha (via `nf-core modules update`) once that PR merges. Thin wrapper around `modkit dmr pair`: compares methylation between a pair of bgzip+tabix bedMethyl inputs over a set of regions and reports differentially methylated regions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
--skip_dmr (default false, only takes effect when --modkit_phased is also set). dmr_cpg_islands_bed/dmr_gencode_gene_bed per genome in igenomes.config, both genome-wide, pre-cleaned (bin column stripped from UCSC's cpgIslandExt dump; gene-level BED derived from GENCODE/CAT-Liftoff GTFs) and chr-named consistently -- see the add-dmr-reference-fixtures branch on IntGenomicsLab/test-datasets (pending PR) for full provenance. Temporarily points at the YannVRB fork branch, to be re-pointed at IntGenomicsLab/test-datasets/main/... once that PR merges (marked with TODOs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
…pendencies Wires the vendored modkit/dmr module into a new subworkflows/local/dmr.nf: MODKIT_PILEUP's phased hp1/hp2 bedMethyl -> HTSLIB_BGZIPTABIX -> CpG-island dual-haplotype-coverage restriction (new local DMR_HAPLOTYPE_REGIONS module, porting NCP's dmr_calling.sh pattern) -> MODKIT_DMR -> nearest-gene annotation via bedtools closest (new local DMR_NEAREST_GENE module). Runs automatically whenever --modkit_phased is set, gated by a new --skip_dmr. Adds GRCh38/CHM13 dmr_cpg_islands_bed/dmr_gencode_gene_bed igenomes.config entries (currently pointing at the IntGenomicsLab/test-datasets fork branch pending that PR's merge) and vendors three canonical nf-core/modules (htslib/bgziptabix, bedtools/intersect, bedtools/closest) plus the not-yet-merged modkit/dmr module. Fixes found via local testing (not committed as a pipeline-level nf-test yet -- a properly-sourced, public methylation-tagged fixture is still needed; none of lrsomatic's existing CI BAMs carry any MM/ML tags): - HTSLIB_BGZIPTABIX output filename collision between a sample's hp1/hp2 outputs -- ext.prefix now includes meta.haplotype. - DMR_NEAREST_GENE's `sort -k1,1V` isn't supported by BusyBox sort (bundled in the bedtools:2.31.1 biocontainer, which ships a coreutils-minimal sort) -- switched to plain `-k1,1 -k2,2n`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
Cosmetic only -- IntGenomicsLab/test-datasets#4 (add-dmr-reference-fixtures) is open but not yet merged, so the URLs themselves are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
…asets IntGenomicsLab/test-datasets#4 merged -- dmr_cpg_islands_bed/dmr_gencode_gene_bed for both GRCh38 and CHM13 now point at IntGenomicsLab/test-datasets/main instead of the YannVRB fork branch. Verified all four URLs resolve (HTTP 200) before committing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
ljwharbers
left a comment
There was a problem hiding this comment.
Thanks for this, @YannVRB. Haplotype DMR is a good fit for --modkit_phased, and the channel wiring holds up. I checked that the _hp1/_hp2.bed.gz selection matches MODKIT_PILEUP's phased output names, that the haplotype tag is added to and then stripped from meta, and that the join/multiMap afterwards is correct. I also downloaded all four reference BEDs. They use chr naming, and the GENCODE files are sorted lexicographically, which matches the sort -k1,1 -k2,2n in DMR_NEAREST_GENE. Thanks also for re-pointing the reference BEDs at IntGenomicsLab/test-datasets/main in a39317d. All four URLs resolve.
I'm requesting changes for the items below. The main problem is that the step runs by default whenever --modkit_phased is set, yet CI never runs it and it can crash on inputs that currently work.
Blocking
1. CI never ran any of the new code, and the vendored modkit/dmr test can't pass.
nf-test-changes found the modkit/dmr, htslib/bgziptabix, bedtools/closest and bedtools/intersect tests, but the PR run filters on --tag small, which drops all four. That's why CI is green: none of them ran. You closed nf-core/test-datasets#2289 in favour of generating the fixture in-test, which works upstream (nf-core/modules#13021 at d886465 now runs a setup { run("MODKIT_PILEUP") } on test.sorted.phased.bam). But the copy vendored here is still pinned at 9d93dfa, and it reads genomics/homo_sapiens/nanopore/bedmethyl/test_hp1.bed.gz, which returns 404. Please re-vendor from the current #13021 head and update the git_sha in modules.json. Since manascripts has just asked for changes upstream, the best time is after you've addressed that. Their point, that the log output isn't deterministic, matches the test.log md5 I was going to raise. Asserting process.out.log.size() == 1 and snapshotting the other outputs, as they suggest, fixes it in both places.
2. We can have a pipeline-level test now: a public phased, methylation-tagged fixture already exists.
genomics/homo_sapiens/nanopore/bam/test.sorted.phased.bam is on nf-core/test-datasets, and this repo already uses it in the MODKIT_PILEUP --phased --modified-bases 5mC 5hmC module test. Your updated upstream module test already shows the pattern: a setup { run("MODKIT_PILEUP") } on that BAM. A subworkflows/local/dmr.nf nf-test that does the same, with a small chr22 CpG-island BED and a gene BED, tagged small, would cover the whole chain in PR CI. I'd rather have that in this PR than as a follow-up.
3. With --modkit_phased, any genome other than GRCh38/CHM13 hits a hard failure.
getGenomeAttribute('dmr_cpg_islands_bed') returns null for a custom --fasta/--genome, and file(null, checkIfExists: true) then fails. Users who never asked for DMR get this, because the step is on by default. The two params also aren't declared in nextflow.config or nextflow_schema.json, so users can't point to their own BEDs. Suggested fix: declare dmr_cpg_islands_bed / dmr_gencode_gene_bed as params with schema entries, let an explicit value override the genome default, and skip DMR with a log.warn (or give a clear error) when neither is available.
4. Default modkit_args vs DMR on dual-modification data.
Your follow-up notes say the default --cpg --modified-bases 5mC is incompatible with modkit dmr pair on real 5mC+5hmC SUP data. If that's true, most --modkit_phased runs on standard ONT data would now fail, or silently produce something wrong, unless the user knows to pass --skip_dmr. What exactly goes wrong, and which modkit_args did your end-to-end run use? I think this should be fixed in this PR, for example by deriving the MODKIT_DMR args from the pileup args. The alternative is to make DMR opt-in until it's fixed.
Should fix
modules.jsonstill points atYannVRB/modules@add-modkit-dmr. Vendoring ahead of nf-core/modules#13021 is fine. Please just keep thegit_shain sync with that PR (see 1) and switch it to nf-core once it merges.- The vendored
bedtools/closestandbedtools/intersectare never used. They add about 790 lines, including tests and snapshots. Either useBEDTOOLS_CLOSESTin place of the local DMR_NEAREST_GENE, which is onlysortplusbedtools closest, or remove both modules and theirmodules.jsonentries. - DMR_HAPLOTYPE_REGIONS unpacks and sorts whole-genome bedMethyl twice (see the inline comment).
- Docs:
docs/usage.md(--skip_dmrand the reference BEDs),docs/output.md(methylation/<type>/dmr/*.nearestGene.bedand its columns: 17 modkit dmr columns, then the gene BED6, then the distance), and a CHANGELOG entry.
Nits
-D ain DMR_NEAREST_GENE (see inline comment).- MODKIT_DMR uses the 0.6.1 biocontainer, while MODKIT_PILEUP uses the patched 0.6.4 image. That's harmless, since DMR only reads bedMethyl, but a one-line comment would stop someone from "aligning" them later.
- MODKIT_DMR gets
--refwithout a.fai. Please check whether modkit re-indexes the 3 GB FASTA in every task. If it does, passfaithrough. - The GENCODE BEDs contain duplicate records at identical coordinates (e.g.
hsa-mir-1253/MIR1253) and some bareENSG…names. With-t first, which of the duplicates is reported depends on file order. - The checklist mentions a "2.14.1 linter bug", but this repo is on template 4.1.0 and the CI
nf-corelint job passed. You can probably tick that box. - Consider publishing the MODKIT_DMR log, for debugging.
| MODKIT_PILEUP.out.bedgz, | ||
| ch_fasta, | ||
| ch_fai, | ||
| file(params.dmr_cpg_islands_bed, checkIfExists: true), |
There was a problem hiding this comment.
For any genome that isn't GRCh38/CHM13 (custom --fasta/--genome), params.dmr_cpg_islands_bed is null here, so file(null, checkIfExists: true) fails the run for every --modkit_phased user. Can you declare both params in nextflow.config and the schema, let an explicit value override the genome default, and skip DMR with a log.warn (or raise a clear error) when neither is set?
| zcat ${hp1_bedmethyl} | awk 'BEGIN{OFS="\\t"}{print \$1,\$2,\$3}' | sort -k1,1 -k2,2n -u > hp1.cpg.bed | ||
| zcat ${hp2_bedmethyl} | awk 'BEGIN{OFS="\\t"}{print \$1,\$2,\$3}' | sort -k1,1 -k2,2n -u > hp2.cpg.bed | ||
|
|
||
| bedtools intersect -u -a ${cpg_islands_bed} -b hp1.cpg.bed | sort -k1,1 -k2,2n > islands.hp1.bed | ||
| bedtools intersect -u -a ${cpg_islands_bed} -b hp2.cpg.bed | sort -k1,1 -k2,2n > islands.hp2.bed | ||
| bedtools intersect -u -a islands.hp1.bed -b islands.hp2.bed | bedtools sort -g genome.txt -i - > ${prefix}.regions.bed |
There was a problem hiding this comment.
On a whole-genome run this zcat | awk | sort -us tens of millions of CpG rows per haplotype. As the PR notes, the container ships BusyBox sort, which sorts in memory, and the process is process_single (6 GB). bedtools intersect -u doesn't need sorted input, and the staged .tbi files are never used. tabix -R ${cpg_islands_bed} hp1.bed.gz (islands sorted), or bedtools intersect -sorted -g genome.txt -u -a islands -b hp1.bed.gz, would stream the data instead of loading it all into memory.
Also, "covered" currently means at least one bedMethyl row in each haplotype, at any depth. Should there be a minimum valid coverage (column 10)?
| task.ext.when == null || task.ext.when | ||
|
|
||
| script: | ||
| def args = task.ext.args ?: '-D a -t first' |
There was a problem hiding this comment.
The DMR regions come from unstranded CpG islands, so modkit's column 6 (strand) is ., and -D a doesn't do anything meaningful. -D b, which reports distance relative to the gene's strand so negative means upstream of the gene, is probably what you want. Please document the sign convention in output.md.
| "git_sha": "feef37435aea56816adf4b3bde1fc76aac327a8d", | ||
| "installed_by": ["modules"] | ||
| }, | ||
| "bedtools/closest": { |
There was a problem hiding this comment.
bedtools/closest and bedtools/intersect are installed but nothing includes them. Either use BEDTOOLS_CLOSEST in place of DMR_NEAREST_GENE, or drop both modules and these entries.
| """ | ||
| input[0] = [ | ||
| [ id: 'test' ], | ||
| file(params.modules_testdata_base_path + 'genomics/homo_sapiens/nanopore/bedmethyl/test_hp1.bed.gz', checkIfExists: true), |
There was a problem hiding this comment.
This fixture returns 404: nf-core/test-datasets#2289 was closed, and upstream #13021 now generates the bedMethyl in-test with setup { run("MODKIT_PILEUP") } on test.sorted.phased.bam. This vendored copy (9d93dfa) predates that change. Please re-vendor from the current #13021 head. When you do, also pick up the fix for the non-deterministic log snapshot that manascripts asked for upstream. PR CI didn't catch this because the --tag small filter skips this test.
… modules Per ljwharbers's review of PR IntGenomicsLab#204: Re-vendored modkit/dmr from nf-core/modules#13021's current head (d886465c) -- the previously-vendored test still pointed at test_hp1.bed.gz/test_hp2.bed.gz, which 404s now that the upstream PR moved to generating its bedmethyl fixture in-test instead. This is why CI never actually exercised the new code. git_sha in modules.json updated to match. bedtools/closest and bedtools/intersect were vendored but never included anywhere -- the local DMR modules shell out to the bedtools CLI directly instead of delegating to these processes. Removed both, plus their modules.json entries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
… when unset Per ljwharbers's review: with --modkit_phased, any genome other than GRCh38/CHM13 hit a hard failure, because these two params only ever got a value via getGenomeAttribute() -- there was no schema entry, no nextflow.config default, and (the real bug) no way for an explicit --dmr_cpg_islands_bed/--dmr_gencode_gene_bed to survive: getGenomeAttribute() unconditionally overwrote whatever the user passed. - nextflow.config: both default to null. - nextflow_schema.json: both declared under reference_genome_options as optional file-path params. - workflows/lrsomatic.nf: params.dmr_cpg_islands_bed ?: getGenomeAttribute(...) instead of a bare overwrite, matching the 'explicit wins' pattern already used for VEP plugin resources in this same file. The DMR invocation block now checks both are present before running DMR; when they're not (no built-in default and no override), it logs a warning and skips DMR instead of crashing the run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
…eshold Per ljwharbers's review: on a whole-genome run, decompressing and sort -u'ing every bedMethyl row genome-wide (tens of millions of rows per haplotype) to find CpG-island overlaps is memory- and IO-heavy. Both hp1/hp2_bedmethyl are already bgzip+tabix indexed by the time this module sees them (HTSLIB_BGZIPTABIX runs just before it) -- query positions inside the CpG islands directly via tabix -R instead, which only reads the rows that can possibly matter. Also addresses the reviewer's second question on this module: 'covered' previously meant any bedMethyl row at all, regardless of depth. Added a minimum valid-coverage threshold (bedMethyl column 10, Nvalid_cov), configurable via ext.args and defaulting to 1 (unchanged behaviour) -- the actual right default for production use is a domain call for the maintainers, not something to silently pick here. Container/environment.yml updated: plain bedtools:2.31.1 has no tabix binary. Reused nf-core/modules' pints/caller container (real, in production use, confirmed via its own environment.yml to bundle both bedtools and htslib) rather than hand-construct an unverifiable Wave/mulled tag. Verified via the new subworkflows/local/tests/dmr.nf.test: real tabix -R output, versions.yml correctly reports both bedtools 2.31.1 and tabix 1.21. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
Per ljwharbers's review: -D a reports signed distance relative to the input (DMR regions, unstranded, always '.') rather than the reference (genes, which do have a meaningful strand) -- -D b is the flag that actually makes the sign meaningful here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
Per ljwharbers's review: a public, phased, methylation-tagged fixture already exists (nf-core/test-datasets' test.sorted.phased.bam, the same one modkit/pileup's and modkit/dmr's own module tests use) -- no need for confidential data or a new fixture to get real CI coverage of the full chain. Chains MODKIT_PILEUP (--phased --modified-bases 5mC 5hmC) -> HTSLIB_BGZIPTABIX -> DMR_HAPLOTYPE_REGIONS -> MODKIT_DMR -> DMR_NEAREST_GENE via a setup block, against a small synthetic chr22:0-40001 CpG-island/gene-model pair (the exact region modkit/dmr's own module test already proves has real dual-haplotype coverage). Tagged 'small' -- .github/workflows/nf-test.yml only runs 'small'-tagged tests on pull_request, so an untagged test would never actually run in CI, which was the reviewer's core point (this exact class of gap already happened once with the vendored module test). Needed a dedicated subworkflows/local/tests/nextflow.config: this subworkflow-level nf-test session does not reliably pick up conf/modules.config's DMR-scoped withName blocks (confirmed directly -- HTSLIB_BGZIPTABIX's ext.prefix/ext.args2 and MODKIT_DMR's ext.args did not apply even with the 'DMR:' prefix present in the process name), so the real, non-stub behaviour this test exercises is set explicitly rather than inherited. Verified real, non-fabricated output: real per-haplotype methylation counts/fractions/deltas from the public BAM, correctly annotated with the test's synthetic gene entry -- not just workflow.success. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
…it as pileup Per ljwharbers's review nits: - "MODKIT_DMR gets --ref without a .fai. Please check whether modkit re-indexes the 3 GB FASTA in every task." Confirmed by direct testing: modkit dmr pair has no --fai flag, looks for <fasta>.fai next to --ref automatically, and builds one from scratch if it's not there (mtime proof: with a pre-built .fai present, it's read unchanged rather than rewritten). Re-vendored modkit/dmr from nf-core/modules#13021 (now fixed upstream) to accept fai as a third element of its fasta input tuple; subworkflows/local/dmr.nf now joins fasta.first() with fai.first() before calling MODKIT_DMR, instead of passing fasta alone. - "Container version mismatch: MODKIT_DMR uses 0.6.1 vs PILEUP's 0.6.4." MODKIT_PILEUP already runs a patched modkit build (ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-6e0afa2, see its own modkit-pileup.diff) for unrelated PacBio pileup fixes; MODKIT_DMR was still on the stock 0.6.1 biocontainer. Patched to the same build for one consistent modkit binary across the whole DMR chain, following the exact same pattern (container swap + a conda/mamba guard, since the patch only exists in the container) already established for pileup. Recorded as modules/nf-core/modkit/dmr/modkit-dmr.diff + a "patch" entry in modules.json, matching pileup's own convention for a locally-modified vendored module. Verified: DMR_NEAREST_GENE output is byte-identical to the unpatched run (the container swap doesn't change dmr pair's calculation, as expected -- the PacBio fix is pileup-specific), and the fai is confirmed staged correctly (genome.fasta.fai symlinked next to genome.fasta in the task work dir). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
PR checklist
modules/nf-core/modkit/dmr, real, non-stub, uses a public nf-core/test-datasets fixture; see below).nf-core pipelines lint(known 2.14.1 linter bug on this checkout, reproduces identically on the pre-existing modkit/pileup module, not something this PR introduces).What
Adds haplotype-vs-haplotype differential methylation region (DMR) calling, discussed with @ljwharbers: when
--modkit_phasedis set, lrsomatic now automatically runs a DMR comparison between a sample's two haplotypes at CpG islands, annotated with the nearest gene. Tumor/normal DMR is explicitly out of scope for this PR (deferred; cross-sample DMR stays in a downstream consumer pipeline).New subworkflow
subworkflows/local/dmr.nf:MODKIT_PILEUP (hp1/hp2 bedMethyl)
-> HTSLIB_BGZIPTABIX (tabix-index each haplotype)
-> DMR_HAPLOTYPE_REGIONS (restrict to CpG islands covered in both haplotypes; new local module, ports the NCP downstream pipeline's dmr_calling.sh pattern)
-> MODKIT_DMR (nf-core modkit/dmr, vendored ahead of upstream merge -- nf-core/modules#13021)
-> DMR_NEAREST_GENE (nearest-gene annotation via bedtools closest; new local module)
Runs automatically whenever
--modkit_phasedis set; a new--skip_dmr(defaultfalse) turns it off independently.New reference data
dmr_cpg_islands_bed/dmr_gencode_gene_bedadded toconf/igenomes.configfor both GRCh38 and CHM13. Currently point at theIntGenomicsLab/test-datasetsfork branch (YannVRB/test-datasets@add-dmr-reference-fixtures) pending that PR's merge -- TODO comments mark where to re-point once it lands.Testing
modules/nf-core/modkit/dmr/tests/): real, non-stub nf-test against a public bedMethyl fixture in nf-core/test-datasets (Add modkit dmr bedmethyl nf-core/test-datasets#2289), itself derived from an existing, already-merged nf-core fixture BAM. Passes.Bugs found and fixed via real end-to-end testing (not stubs)
HTSLIB_BGZIPTABIXoutput filename collision between a sample's hp1/hp2 outputs --ext.prefixnow includesmeta.haplotype`.DMR_NEAREST_GENE'ssort -k1,1Visn't supported by BusyBox sort (bundled in thebedtools:2.31.1biocontainer, which ships a coreutils-minimal sort) -- switched to plain-k1,1 -k2,2n.Follow-ups (not blocking, tracked separately)
modkit_args(--cpg --modified-bases 5mC) is incompatible withmodkit dmr pairagainst real dual-modification (5mC+5hmC) SUP-basecalled data -- see maintainer report.modules.jsonandconf/igenomes.configat the canonical locations.