Skip to content

Add hostile as alternative decontamination tool#712

Open
jfy133 wants to merge 17 commits into
devfrom
add-hostile
Open

Add hostile as alternative decontamination tool#712
jfy133 wants to merge 17 commits into
devfrom
add-hostile

Conversation

@jfy133

@jfy133 jfy133 commented Mar 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Transferred from #646

TODO:

  • Verify input check logic makes sense
  • Find test work around (preprovided refs. currently requires too much memory)

PR checklist

  • 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 pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/taxprofiler branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@jfy133 jfy133 marked this pull request as draft March 27, 2026 05:38
@github-actions

github-actions Bot commented Mar 27, 2026 •
edited
Loading

Copy link
Copy Markdown

nf-core pipelines lint overall result: Passed ✅

Posted for pipeline commit 15b3c37

+| ✅ 339 tests passed       |+
#| ❔   3 tests were ignored |#
#| ❔   1 tests had warnings |#
Details

❔ Tests ignored:

  • files_exist - File is ignored: conf/igenomes.config
  • files_exist - File is ignored: conf/igenomes_ignored.config
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md

❔ Tests fixed:

✅ Tests passed:

Run details

  • nf-core/tools version 4.0.2
  • Run at 2026-06-16 07:26:45

@jfy133

jfy133 commented Mar 27, 2026

Copy link
Copy Markdown
Member Author

I realised the reason why the test didn't work as expected is because I Forgot to supply a reference genome in the test confrig, so it tried to download instead 🤦‍♂️.

I've requested a small test dataset on the hostile reop in the meantime (bede/hostile#70), but also can try again but supplying the small reference genome we use for host removal explicitly and let it be built on the fly, rather than testing download

@bede

bede commented Mar 27, 2026

Copy link
Copy Markdown

As requested by James I've added a new mitochondrial index to Hostile's storage bucket to facilitate testing requested here
bede/hostile#70

@jfy133

jfy133 commented Mar 28, 2026

Copy link
Copy Markdown
Member Author

Started adding tests thanks to @bede ! I need to clean up the code logic a bit, in particular work out what validation to do (reference genome vs index name etc)

@bede

bede commented Mar 28, 2026

Copy link
Copy Markdown

FYI I've renamed the human mitochondrial hostile (and deacon indexes) to test-human-mit so that nobody gets confused

@jfy133

jfy133 commented Mar 31, 2026

Copy link
Copy Markdown
Member Author

Just waiting for @bede to address bede/hostile#71 and then this is ready for review!

@jfy133 jfy133 marked this pull request as ready for review March 31, 2026 13:05
@bede

bede commented Apr 13, 2026

Copy link
Copy Markdown

Just waiting for @bede to address bede/hostile#71 and then this is ready for review!

Sorted I think, please reopen if not :-)

sofstam
sofstam reviewed Apr 30, 2026
View reviewed changes

@sofstam sofstam left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A few comments and would be nice to update to the latest hostile modules.

Comment thread workflows/taxprofiler.nf Outdated
custom_adapters = params.longread_qc_adapterlist ? file(params.longread_qc_adapterlist, checkIfExists: true) : []

if (params.hostremoval_reference) {
if (params.shortread_hostremoval_tool == 'bowtie2' || params.longread_hostremoval_tool == 'bowtie2') {

@sofstam sofstam Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do you mean minimap2 for the long read?

@bede bede Apr 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In case helpful, while Hostile does support using Minimap2 for short reads and Bowtie2 for long reads, this is not recommended, and the CLI defaults steer users away from doing this. If this PR is not doing so already, I strongly suggest setting minimap2 as the aligner for long reads and bowtie2 for short reads, stopping users from making bad choices.

@sofstam sofstam Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

params.longread_hostremoval_tool == 'bowtie2'

I am referring to this statement

@jfy133 jfy133 Apr 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, I think that might be a copy paste error!

@jfy133 jfy133 Apr 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed the longread_hostremoval_tool parameter does not support bowtie2 at all:

taxprofiler/nextflow_schema.json

Line 367 in c2ce5c9

"enum": ["minimap2", "hostile"]

Comment thread workflows/taxprofiler.nf
Comment thread docs/output.md
Comment thread subworkflows/local/longread_hostremoval/main.nf

@sofstam sofstam Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We apply samtools stats for minimap2. Shall we do the same for hostile?

Comment thread subworkflows/local/shortread_hostremoval/main.nf

@sofstam sofstam Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here for the samtools stats

@nf-core-bot

nf-core-bot commented Apr 30, 2026

Copy link
Copy Markdown
Member

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 3.5.2.
Please update your pipeline to the latest version.

For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation.

jfy133
jfy133 commented Jun 16, 2026
View reviewed changes
Comment thread tests/test_alternativepreprocessing.nf.test.snap
Comment thread tests/test_alternativepreprocessing.nf.test.snap
Comment thread tests/test_alternativepreprocessing.nf.test.snap
@jfy133
Apply suggestions from code review
15b3c37 
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bede bede bede left review comments
@sofstam sofstam sofstam left review comments
@LilyAnderssonLee LilyAnderssonLee Awaiting requested review from LilyAnderssonLee LilyAnderssonLee is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

size/xl

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jfy133 @bede @nf-core-bot @sofstam