Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MaxBin2 abundance input #690

Merged
merged 10 commits into from
Oct 18, 2024
Merged

Conversation

d4straub
Copy link
Collaborator

@d4straub d4straub commented Oct 11, 2024

Fix #663

I adopted for CONVERT_DEPTHS largely the solution by @uel3 but changes the MaxBin2 module so that it accepts depth files via -abund, I think this is a more elegant solution. Additionally the CONVERT_DEPTHS module was changed so that the abundance files are named by samples, that makes the *.abundance files by MaxBin2 include the sample names, i.e. they are meaningful now.
The nf-core module maxbin2 was updated separately and is used here.
Linting with nf-core tools version 2.14.1 succeeds.

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/mag branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core 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).

@d4straub d4straub changed the title Fix maxbin2 multiabund Fix MaxBin2 abundance input Oct 11, 2024
@jfy133 jfy133 marked this pull request as draft October 12, 2024 10:54
@jfy133
Copy link
Member

jfy133 commented Oct 12, 2024

I would rather these to go in the official module @d4straub but I'm around for fast review for that too

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.

Looking good so far, just I would request porting this to the official module (like I said above, I'm available for rapid review on that)

My last question is (being naive but want to make sure this is recorded for future me):

I was just thinking @uel3 mentioned this issue in the context of co-assembly, do we need to distinguish the mechanism being added here between standalone or co-assembly?

Reading the minimal Maxbin2 docs I think it doesn't really matter - so we can always do it in this way, but I just want to double check as you likely have more experience than I have with the tool.

modules/nf-core/maxbin2/main.nf Show resolved Hide resolved
@d4straub
Copy link
Collaborator Author

Looking good so far, just I would request porting this to the official module (like I said above, I'm available for rapid review on that)

Thasnks, PR to modules is open.

I was just thinking @uel3 mentioned this issue in the context of co-assembly, do we need to distinguish the mechanism being added here between standalone or co-assembly?

Not sure I understand the question. In my tests this works with and without co-assembly, it depends rather on --binning_map_mode, and it works in my tests as expected.

@jfy133
Copy link
Member

jfy133 commented Oct 14, 2024

Looking good so far, just I would request porting this to the official module (like I said above, I'm available for rapid review on that)

Thasnks, PR to modules is open.

OK will look now!

I was just thinking @uel3 mentioned this issue in the context of co-assembly, do we need to distinguish the mechanism being added here between standalone or co-assembly?

Not sure I understand the question. In my tests this works with and without co-assembly, it depends rather on --binning_map_mode, and it works in my tests as expected.

That answers my question basically 👍

@jfy133
Copy link
Member

jfy133 commented Oct 14, 2024

@d4straub module can be installed in this PR now :)

@d4straub d4straub marked this pull request as ready for review October 14, 2024 11:05
@d4straub d4straub requested a review from jfy133 October 14, 2024 11:24
@jfy133
Copy link
Member

jfy133 commented Oct 14, 2024

Just going to run a test with current version and your changes to compare, but code looks good 👍

@jfy133
Copy link
Member

jfy133 commented Oct 15, 2024

@d4straub I had a look and it seems to run through, however when I run with test_virus_identification which does co-assembly, the depths/bin_summary.tsv only reports CONCOCT... do you see that in your test run also?

@d4straub
Copy link
Collaborator Author

d4straub commented Oct 16, 2024

@jfy133 I checked the code and CONVERT_DEPTHS is only channeled into ch_maxbin2_input which only goes into MAXBIN2. As far as I see, of the many files that are available from MAXBIN2.out, summary file isnt used, and that file is the only file that changes in this PR (MaxBin2's summary file looses column that has abundance with the fix).

The following works as expected for me (this runs on this modified branch), i.e. GenomeBinning/depths/bins/bin_depths_summary.tsv & GenomeBinning/bin_summary.tsv contain all bins:

nextflow run mag -profile test_bbnorm,singularity --skip_binning false --centrifuge_db false --kraken2_db false --outdir results_test_bbnorm_3-1-0dev -resume --skip_concoct
nextflow run mag -profile test_bbnorm,singularity --skip_binning false --centrifuge_db false --kraken2_db false --outdir results_test_bbnorm_3-1-0dev_concoct -resume

test_virus_identification doesnt produce any binning results, because it uses

    skip_binning                = true
    skip_metabat2               = true
    skip_maxbin2                = true 

so I also would not expect any results from binners. depths/bin_summary.tsv doesnt exist in any case, I think.

Edit: clarity
Edit2: test_virus_identification run through, cannot find a problem, i.e. same output for nf-core/mag -r 3.1.0

@jfy133
Copy link
Member

jfy133 commented Oct 16, 2024

OK that's strange, maybe I just had a weird test configuration (using the test_virus_identification profile, and extra flags), but it looks like you've tested in multiple configurations and it always looks OK 👍

CHANGELOG.md Outdated Show resolved Hide resolved
@jfy133
Copy link
Member

jfy133 commented Oct 16, 2024

@d4straub I will merge in and then do the template merge

@jfy133 jfy133 merged commit 17d87ff into nf-core:dev Oct 18, 2024
14 of 15 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.

2 participants