Skip to content

Conversation

@AlexHoratio
Copy link
Contributor

Happy Yuletide, developers of nf-core! 😊

As a gift I bring you this PR: MetaBinner/SemiBin2 should be included as inputs to DASTool when available. I believe this fixes it by replicating the existing pattern for other binners (just tested on my own data, appears to work).

PR checklist

  • This comment contains a description of changes (with reason).

Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Thank you @AlexHoratio !

I've not tested, but looks simple enough to me. I'll let someone else from @nf-core/nf-core-mag to double check :)

@jfy133
Copy link
Member

jfy133 commented Jan 5, 2026

of the tests:

  1. Is checkM2 database download time out
  2. The second is a a snapshot update required for assembly_input - @AlexHoratio you will need t orun nf-test test --tag test_assembly_input --profile +docker --update-snapshot (or something along this lines), to incorporate teh FASTA2CONTIGBIN output for semi2bin into the sanpshot :)

@prototaxites
Copy link
Contributor

This looks fine to me also, but I just want to double-check - are the contig headers of the bins from these new binners compatible with DAS_Tool? Some binners for whatever reason rename contigs, but these bin refinement tools need the original contig names to remain in place.

@AlexHoratio
Copy link
Contributor Author

This looks fine to me also, but I just want to double-check - are the contig headers of the bins from these new binners compatible with DAS_Tool? Some binners for whatever reason rename contigs, but these bin refinement tools need the original contig names to remain in place.

I just double-checked some of my outputs, and I can't find any evidence of the contigs being renamed, and it does seem to mix with DASTool appropriately.

Also, that snapshot update test is still failing... I think that's my fault for trying to run it on my under-powered local machine. I'll try it again in a moment.

@jfy133
Copy link
Member

jfy133 commented Jan 5, 2026

Don't worry @AlexHoratio the failing test isn't you

image

Zenodo is 403ing for some reason when downloading the CheckM2 database.. not to do with you I think :)

@dialvarezs
Copy link
Member

Zenodo is 403ing for some reason when downloading the CheckM2 database.. not to do with you I think :)

It seems that Zenodo is blocking aria2 UA 🙃. It doesn't work for me either, but it does when changing UA, e.g. --user-agent="Wget/1.21.4" .

@jfy133
Copy link
Member

jfy133 commented Jan 20, 2026

@AlexHoratio sorry taken so long to get to this. We refactored somewhat the DAS_Tool steps in another PR, so there were some merge conflicts I've tried to resolve. Please have a look to make sure I didn't make sure

Otherwise I think the nf-test snapshot may still need updating, but we w ill see

(checkM issues should be resolved now!)

@AlexHoratio
Copy link
Contributor Author

Awesome, well that is a good refactor 😊 I believe the test_assembly_input snapshot should now be updated again.

@jfy133
Copy link
Member

jfy133 commented Jan 21, 2026

Is that something you have capacity to do, or should one someone take over @AlexHoratio ?

@AlexHoratio
Copy link
Contributor Author

If you just need me to test the pipeline on some real data and see if this PR currently works, I have just done that, and it appears to do so.. all binners still occur appropriately in DASTool outputs.

Anything else may lie beyond my Nextflow know-how 🥲

@jfy133
Copy link
Member

jfy133 commented Jan 21, 2026

Works for me! Thank you @AlexHoratio ! LGTM!

@jfy133 jfy133 self-requested a review January 21, 2026 13:57
@jfy133 jfy133 merged commit e355698 into nf-core:dev Jan 21, 2026
8 checks passed
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