Skip to content

new module: rastair2/call - #13610

Open
eduard-watchmaker wants to merge 4 commits into
masterfrom
rastair2-call
Open

eduard-watchmaker wants to merge 4 commits into
masterfrom
rastair2-call

Conversation

@eduard-watchmaker

@eduard-watchmaker eduard-watchmaker commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Add RASTAIR2_CALL for rastair version 2 (bioconda::rastair=2.2.0), which jointly calls SNVs, indels and CpG methylation from 5mC->T converted reads (TAPS/TAPS+, Illumina 5-base).

  • Inputs: [meta, bam, bai], [meta2, fasta, fai].
  • Outputs: bgzipped VCF (+ tabix index; rastair 2.2.0 writes none) and the CpG methylation BED (+ tbi). Extra options go through ext.args (e.g. --guess-read-orientation for non-directional libraries).
  • meta.single_end adds --unpaired so strands come from alignment direction.
  • Separate from rastair/* (0.8.x, GPL-3.0) because rastair >=2 has a non-commercial licence; meta.yml documents the terms and the Oxford University Innovation contact for commercial use.
  • Seqera containers (docker/singularity, amd64/arm64) built via nf-core modules containers create.
  • Tests: paired-end, single-end (asserts --unpaired reached rastair), stub. The VCF body is hashed as text because rastair declares Number=M FORMAT fields (VCF 4.4+), which the htsjdk-based nft-vcf parser rejects.

PR checklist

Closes #XXX

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Broadcast software version numbers to topic: versions - See version_topics
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

Add RASTAIR2_CALL for rastair version 2 (bioconda::rastair=2.2.0), which
jointly calls SNVs, indels and CpG methylation from 5mC->T converted reads
(TAPS/TAPS+, Illumina 5-base).

- Inputs: [meta, bam, bai], [meta2, fasta, fai].
- Outputs: bgzipped VCF (+ tabix index; rastair 2.2.0 writes none) and the
  CpG methylation BED (+ tbi). Extra options go through ext.args
  (e.g. --guess-read-orientation for non-directional libraries).
- meta.single_end adds --unpaired so strands come from alignment direction.
- Separate from rastair/* (0.8.x, GPL-3.0) because rastair >=2 has a
  non-commercial licence; meta.yml documents the terms and the Oxford
  University Innovation contact for commercial use.
- Seqera containers (docker/singularity, amd64/arm64) built via
  `nf-core modules containers create`.
- Tests: paired-end, single-end (asserts --unpaired reached rastair), stub.
  The VCF body is hashed as text because rastair declares Number=M FORMAT
  fields (VCF 4.4+), which the htsjdk-based nft-vcf parser rejects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/l label Nov 4, 2026
@piplus2

piplus2 commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Hi @eduard-watchmaker, and thanks for the contribution! 🎉 This is a well-built module. The topic: versions outputs, stub test, container metadata and meta.yml docs are all in good shape. I have a few things to tidy up against the nf-core conventions, plus one point for the maintainers.

Things to change

  1. environment.yml should list tabix explicitly. main.nf calls tabix and reports its version. Right now it only arrives as a transitive dependency of rastair, which is fragile. Please add bioconda::htslib=<version> (the version the container currently resolves, 1.24) so every tool the module uses is declared. Then rebuild the containers with nf-core modules containers create and refresh the meta.yml containers: block and the conda-lock files.

  2. Remove the copyright/licence header at the top of main.nf. nf-core modules don't carry per-file headers, because the repo's MIT licence covers them. The Oxford non-commercial notice is useful, but meta.yml is the right place for it, and you already have it there.

  3. CRAM is documented but not tested. The bam input says "BAM or CRAM", but only BAM is tested. Either add a CRAM test, or narrow the docs to BAM. For CRAM, the test would also confirm the fasta and fai inputs work for reference decoding.

  4. The single-end test reuses a paired-end BAM. That's fine for checking --unpaired reaches rastair, and your assertion on ##rastairCommand= is a nice touch. A short comment in the test saying this is deliberate would help future readers. Using genuinely single-end test data would be better if any exists.

  5. PR description. The checklist is still unticked and Closes #XXX is still the placeholder. Please tick what you've done (including the docker, singularity and conda test runs) and delete the lines that don't apply.

Smaller suggestions (optional)

  • The bai input also accepts .csi and .crai. Naming it index would be more accurate, but bai is common in nf-core, so this is your call.
  • The description: in meta.yml is long because of the licence text. Consider keeping the description short and putting the licensing detail in a tools description or a note.
  • Add a short mention in meta.yml that the fai is staged next to the FASTA and not passed on the command line. This is standard nf-core practice, but one sentence makes it clear.

For the maintainers

  • Licence. LicenseRef-NonCommercial isn't an SPDX identifier, and this would be one of the few non-open-source tools in the repo. Could a core team member confirm that's acceptable? If it is, they should also confirm the licence string format that module lint expects.
  • Naming. rastair2/call sits next to the existing rastair/* (0.8.x) modules. The reason for the split is clear and well explained in the description, since the licences differ. Just check that a version number in the tool name is the preferred pattern here.

Thanks again, and ping me when the changes are in! 🙌

- Declare htslib (tabix) in environment.yml and rebuild containers.
- Drop the per-file header; licence terms stay in meta.yml.
- Rename the bai input to index (bai, csi or crai).
- Add a CRAM test and use genuinely single-end reads for the single-end test.
- Shorten the meta.yml description and document fai staging.

Generated by Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@eduard-watchmaker

Copy link
Copy Markdown
Contributor Author

Hey @piplus2 thanks so much for putting the time into reviewing this PR and your insights!
I will adddress your points one by one:

  1. Added bioconda::htslib=1.24 and rebuilt the containers; meta.yml and the conda-lock files are refreshed. 1.24 is the newest version rastair 2.2.0 allows (htslib >=1.24,<1.25), so lint’s suggestion of 1.25 doesn’t apply.
  2. Removed the header. The licence terms stay in meta.yml.
  3. Added a CRAM test (homo_sapiens chr22 CRAM with genome.fasta and .fai). rastair decodes it using only the reference inputs, and the test asserts the VCF has records.
  4. The single-end test now uses test.single_end.sorted.bam, which is genuinely single-end data. Unconverted reads are valid TAPS input: they read as unmethylated.
  5. Description updated.

Also, I renamed bai to index, moved the licensing text into the tools description, and added a note that the fai is staged next to the FASTA.

@ewels confirmed on Slack [https://nfcore.slack.com/archives/CP3RJSMF0/p1793748772262849] that the license is acceptable (link the thread). Versioned tool names have plenty of precedent (bwamem2, bwamem3, gatk4, kraken2); here the split is because 0.8.x and 2.x have different licences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants