Skip to content

docs(modkit/pileup): clarify BAM index, BED format, and --cpg requirements - #10341

Open
sahuno wants to merge 3 commits into
nf-core:masterfrom
sahuno:fix/modkit-pileup-meta-docs
Open

docs(modkit/pileup): clarify BAM index, BED format, and --cpg requirements#10341
sahuno wants to merge 3 commits into
nf-core:masterfrom
sahuno:fix/modkit-pileup-meta-docs

Conversation

@sahuno

@sahuno sahuno commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Update BAM index pattern from *.bai to *.{bai,csi} — modkit supports CSI indices via htslib, and ONT BAMs commonly use CSI
  • Document that --include-bed requires strict BED3 or BED6 format — BED4 (e.g. UCSC CpG island downloads) is silently rejected, leading to a confusing "zero valid positions parsed" error
  • Note that --cpg requires --modified-bases to also be passed via ext.args, otherwise modkit errors with "required arguments not provided"

All three issues were discovered while testing the module with real ONT methylation data (mm10, dorado basecalled BAMs with 5mCG_5hmCG modifications).

Changes

Documentation-only changes to meta.yml. No code changes to main.nf.

Test plan

  • Verified module runs successfully with CSI-indexed BAMs via Nextflow on SLURM
  • Verified --cpg --modified-bases 5mC produces correct bedMethyl output
  • Verified BED3 input works after converting from BED4
  • Existing nf-test suite should pass unchanged (no code modifications)

🤖 Generated with Claude Code

…ments in meta.yml

- Update BAI index pattern to *.{bai,csi} since modkit supports CSI indices via htslib
- Document that --include-bed requires BED3 or BED6 format (BED4 is rejected)
- Note that --cpg requires --modified-bases to be passed via ext.args

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@SPPearce

SPPearce commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Please post on the nf-core slack #github-invitations channel, to be added to the organisation so the tests runs.

@SPPearce SPPearce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it worth adding a check in the module code itself for the --modified-bases or the number of columns in the bed file? Rather than just the meta.yml

@fellen31

fellen31 commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Is it worth adding a check in the module code itself for the --modified-bases or the number of columns in the bed file? Rather than just the meta.yml

For --modified-bases, I think the tool handles that pretty clearly already, i.e.:

error: the following required arguments were not provided:
  --modified-bases <MODIFIED_BASES>...

For better error reporting on the number of columns in the bed file, I feel like this is something that should be adressed in an issue at https://github.com/nanoporetech/modkit rather than checking it in the nf-core module. The tool is actively developed and maintained.

@famosab

famosab commented Apr 2, 2026

Copy link
Copy Markdown
Contributor

Can you open an Issue over at the original repo then? :)

@sahuno

sahuno commented Apr 6, 2026

Copy link
Copy Markdown
Contributor Author

Upstream issue opened at nanoporetech/modkit#605 — reporting that --include-bed silently rejects BED4 files with no warning.

Also now part of the nf-core GitHub org so CI should run on this PR.

@sahuno

sahuno commented Apr 6, 2026

Copy link
Copy Markdown
Contributor Author

@SPPearce @famosab — both blockers are now resolved:

  1. Upstream issue filed at modkit pileup --include-bed silently rejects BED4 files, produces zero valid positions with no warning nanoporetech/modkit#605 (BED4 silent rejection)
  2. Now part of the nf-core GitHub org — CI is passing ✅

Happy to address any further feedback!

@famosab

famosab commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Note that --cpg requires --modified-bases to also be passed via ext.args, otherwise modkit errors with "required arguments not provided"

I think we could add a check in the module main.nf like

def mod_bases_cmd = task.ext.args.contains("--cpg") ? "--cpg --modified-bases": ""

but other than that I think it looks good

But maybe the modified bases can not be deducted from the input then feel free to ignore :)

…e-bed coupling

Verified each claim against modkit 0.6.1 rather than paraphrasing:

- A BED4 `--include-bed` fails with "improperly formatted BED line, must be
  BED3 or BED6" followed by "zero valid positions parsed from BED file";
  BED3 and BED6 both work. Quoting the exact strings makes the note greppable
  when someone hits the error.
- `--include-bed` additionally requires `--motif` or `--modified-bases` in
  ext.args, independently of `--cpg` ("currently, --include-bed requires a
  --motif or --modified-bases"). This coupling was undocumented; added it to
  the `bed` input description.

No `main.nf` change: modkit's own error for a missing `--modified-bases` is
already explicit, and the correct value (5mC, 5hmC, ...) depends on what the
basecaller tagged, so it cannot be deduced from the module inputs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E7RSvpouxbMBdimYSwJ5ZP
@sahuno

sahuno commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@SPPearce @famosab @fellen31 — I went back to modkit 0.6.1 and checked each
of these against the actual CLI rather than reasoning about it. Results:

On adding a --modified-bases check in main.nf: I agree with @fellen31
the tool's own error is already unambiguous and points at exactly the missing
flag, so a module-side check would only duplicate it:

$ modkit pileup --cpg --ref genome.fasta in.bam out.bed
error: the following required arguments were not provided:
  --modified-bases <MODIFIED_BASES>...

@famosab's task.ext.args.contains("--cpg") ? "--cpg --modified-bases" : ""
would also need a value for --modified-bases (5mC, 5hmC, …), and that
can't be deduced from the inputs — the right value depends on what the
basecaller actually tagged. Silently picking one would be worse than the
current error. So I've left main.nf alone and kept this as documentation.

On the BED column count: confirmed, and I've put the exact error text in
meta.yml so it's greppable. A BED4 --include-bed gives:

> improperly formatted BED line, must be BED3 or BED6 chr22	0	40001	region1
> Error! zero valid positions parsed from BED file

BED3 and BED6 both work (1706 and 821 rows respectively on the test BAM).

One extra coupling I found while testing, which wasn't in the original PR
and isn't documented anywhere: --include-bed also requires --motif or
--modified-bases, independently of --cpg:

> Error! currently, --include-bed requires a --motif or --modified-bases.
  This limitation may be removed in the future.

I've added that to the bed input description too, since it's the same class
of footgun as the --cpg one.

Upstream issue for the BED4 handling is at nanoporetech/modkit#605.

Brings the branch up to date with master. The stale base made the
`Get module name` step of the Linting workflow fail with
"Argument list too long": paths-filter diffed 1521 commits' worth of
module files into the inline github-script argv.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013fqrTt7TV2RSSUHaA4wP4v
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