Repository navigation
Add modkit/dmr module - #13021
Open
YannVRB wants to merge 9 commits into
Open
Add modkit/dmr module#13021YannVRB wants to merge 9 commits into
YannVRB wants to merge 9 commits into
Conversation
Thin wrapper around `modkit dmr pair`, comparing methylation between a pair of bgzip+tabix bedMethyl inputs (e.g. two haplotypes, or tumor and normal) and reporting differentially methylated regions. Modeled on modkit/pileup and modkit/callmods' existing conventions in this repo (same container, same versions-topic pattern). Test data (test_hp1.bed.gz/test_hp2.bed.gz, bgzip+tabix'd) generated with modkit pileup --phased --modified-bases 5mC 5hmC on the existing genomics/homo_sapiens/nanopore/bam/test.sorted.phased.bam fixture -- the same inputs and modified-bases args modkit/pileup's own --phased tests already use. Added via a separate PR to nf-core/test-datasets (branch add-modkit-dmr-bedmethyl on YannVRB/test-datasets_YV), not yet merged -- tests/config/nf-test.config here still points at the canonical nf-core/test-datasets modules_testdata_base_path and will resolve once that PR lands. Verified locally by temporarily pointing modules_testdata_base_path at the fork branch: all 3 tests pass against real `modkit dmr pair` output (Nextflow 25.10.2, matching this repo's nf-test.yml CI pin -- 26.04.6 fails nf-test's generated harness under strict syntax, an unrelated tooling issue). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
This was referenced Sep 23, 2026
kubranarci
reviewed
Sep 24, 2026
kubranarci
left a comment
kubranarci
left a comment
Contributor
There was a problem hiding this comment.
Seems pretty good to me, yet I will wait until test-file conflict is resolved.
| description: Optional BED file of regions over which to compare methylation | ||
| levels. When omitted, methylation levels are compared at each site instead | ||
| pattern: "*.bed" | ||
| ontologies: [] |
Addresses kubranarci's review comment on nf-core#13021: bedmethyl in/out files get format_3003 (BED) + format_3989 (GZIP), their tabix indexes get format_3700, the optional regions_bed and reference fasta get format_3003 and format_1929 respectively. log output left as-is (no EDAM term fits an arbitrary tool log). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
…tting one Per kubranarci's review suggestion on nf-core#13021: chain modkit/pileup (--phased, on the existing genomics/homo_sapiens/nanopore/bam/test.sorted.phased.bam fixture) and htslib/bgziptabix (to index modkit pileup's --bgzf output, which is compressed but not indexed) in a setup block, instead of pointing at static test_hp1.bed.gz/test_hp2.bed.gz files. This removes the need for the companion test-datasets PR (nf-core#2289) entirely -- no new binary fixture required, and the test now exercises the exact real-world chain (MODKIT_PILEUP -> HTSLIB_BGZIPTABIX -> MODKIT_DMR) rather than a pre-generated snapshot of it. --modified-bases 5mC 5hmC on the pileup step (matching modkit/pileup's own phased test) avoids a real bug: this BAM's basecalling calls both 5mC and 5hmC, and requesting only one leaves the other counted as "other" in modkit's bedMethyl output, which MODKIT_DMR's internal consistency check rejects as invalid-bedmethyl-data. All 3 tests pass against the real chained output (verified: real per-region haplotype methylation delta values in the resulting .bed, not stub/fixture content). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
kubranarci
reviewed
Sep 24, 2026
| "content": [ | ||
| { | ||
| "0": [ | ||
| [ |
Contributor
There was a problem hiding this comment.
your stub seems to be empty now
The stub variant was silently passing with entirely empty output channels (caught by the user, not by the earlier green checkmark). Root cause: -stub-run makes the setup-chained MODKIT_PILEUP also run in stub mode, and its stub script never splits by haplotype -- it always emits one generic <prefix>.bed.gz, with no _hp1/_hp2 suffix. The setup's regex filter for that suffix therefore matched nothing, so HTSLIB_BGZIPTABIX's setup run had an empty input channel and never executed, leaving MODKIT_DMR's own -stub test with nothing to consume either. Fixed by normalizing MODKIT_PILEUP.out.bedgz's payload to a list first ([x].flatten(), since Nextflow emits a single Path rather than a List when only one file matches the module's output glob -- the normal case for -stub), then falling back to reusing that single stub file for both haplotypes when no _hp[12] suffix is found. MODKIT_DMR's own stub script never reads its inputs, so reusing one file for both bedmethyl_a/b is correct for this case. Verified: the stub test now actually submits HTSLIB_BGZIPTABIX/MODKIT_DMR tasks and produces real (touch'd) output files, not an empty channel. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
kubranarci
approved these changes
Sep 24, 2026
manascripts
requested changes
Sep 28, 2026
Per manascripts's review: modkit dmr pair's log includes a timestamp/runtime that varies between runs, making it unsuitable for an exact-content snapshot. Assert process.out.log.size() == 1 for existence, and snapshot only the deterministic outputs (bed, versions_modkit). Verified: all 3 tests pass against real output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
YannVRB
added a commit
to YannVRB/lrsomatic
that referenced
this pull request
Sep 28, 2026
… 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
manascripts
approved these changes
Sep 29, 2026
Tackles failing lint (still need to update snapshots)
Member
|
Please join the nf-core organization on GitHub to enable the CI-tests to run on your PR. You can request to join the organization via #github-invitations in the nf-core slack. You can join the nf-core slack via https://nf-co.re/join. |
…te snapshots manascripts's fix (25d5ea8) applied process.out.findAll { key, val -> key.startsWith('versions') } to the two real-execution tests but not the third (-stub) test, which still had the old snapshot(process.out.bed, process.out.versions_modkit) form -- same substring-search lint failure would apply there too. Applied the same pattern there, then regenerated all 3 snapshots (their commit had updated the test code but not yet the .snap file, as their message noted). Verified against nf-core/tools' dev branch (same install CI uses, not the stale 2.14.1 in this environment's default conda env, which gives a false-positive CRITICAL 'name' on every module including known-good ones): nf-core modules lint modkit/dmr now passes cleanly, 62/62. All 3 nf-tests still pass against real output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
Per ljwharbers's review of the downstream lrsomatic PR: modkit dmr pair gets --ref with no .fai present, and has no --fai flag of its own -- it looks for <fasta>.fai next to the reference automatically. Confirmed by direct testing: with no .fai present, modkit dmr pair builds one; with one already staged next to fasta, it's read (mtime unchanged) rather than rebuilt. For a several-GB genome, rebuilding that index on every task invocation is a real, avoidable cost. fasta is now a 3-tuple (meta, fasta, fai) instead of 2. fai is never referenced directly in the script -- passing it only makes Nextflow stage it alongside fasta so modkit's own auto-discovery finds it. Pass [] to omit, same as before. Verified: all 3 tests pass against real output (bed content is identical either way, as expected -- fai only affects whether the index gets rebuilt, not the DMR calculation), and nf-core modules lint modkit/dmr passes cleanly (62/62). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wit78XR11V9AMAgY7gGrMT
YannVRB
added a commit
to YannVRB/lrsomatic
that referenced
this pull request
Sep 30, 2026
…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
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
5 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.
PR checklist
Description
Adds modkit/dmr, a thin wrapper around modkit dmr pair: compares methylation between a pair of bgzip+tabix bedMethyl inputs (e.g. two haplotypes from a phased sample, or tumor/normal) over a set of regions, and reports differentially methylated regions. Same container/versions-topic conventions as the existing modkit/pileup and modkit/callmods modules in this repo.
Test data (test_hp1.bed.gz/test_hp2.bed.gz) is added in a companion PR to nf-core/test-datasets: nf-core/test-datasets#2289. That PR needs to merge before this one's CI will be fully green, the test file already points at the canonical modules_testdata_base_path, so nothing here needs to change once it lands, but until then the test-datasets-dependent tests will fail to resolve the fixture URLs.
Locally verified (Nextflow 25.10.2, matching this repo's nf-test.yml pin) by temporarily pointing modules_testdata_base_path at the test-datasets fork branch: all 3 tests (with regions, without regions, stub) pass against real modkit dmr pair output, not just process success.