Skip to content

Conversation

@muabnezor
Copy link
Contributor

@muabnezor muabnezor commented Nov 28, 2024

This PR adds long-read only functionality to mag.

closes #662, #659, #275

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 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).

@muabnezor muabnezor added the WIP Work in progress label Nov 28, 2024
@muabnezor muabnezor requested a review from jfy133 November 28, 2024 12:28
@github-actions
Copy link

github-actions bot commented Nov 28, 2024

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 3.2.1.
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.

@muabnezor muabnezor changed the base branch from master to dev November 28, 2024 12:29
Copy link
Collaborator

@d4straub d4straub left a comment

Choose a reason for hiding this comment

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

I only had a short look, sorry, but I just wanted to express my gratitude that you tackle long read assembly!

@muabnezor
Copy link
Contributor Author

I only had a short look, sorry, but I just wanted to express my gratitude that you tackle long read assembly!

My pleasure hehe. Still WIP. I have to tidy up the code and run some validation on real data, but we're getting there!

@muabnezor
Copy link
Contributor Author

@jfy133 thanks for the review. I think I have addressed your suggested changes. I hope the tests go well!

@prototaxites
Copy link
Contributor

FYI all - we have had a pretty catastrophic outage on our cluster and I'm not currently able to finish the test I was running - and it's unlikely I'll get a chance to finish it any time soon.

But I'm happy with the PR as-is - the long-read bits were working well after we fixed those wee bugs!

@jfy133
Copy link
Member

jfy133 commented May 28, 2025

Thanks for the heads up Jim!

@muabnezor I will check the changes later today or tomorrow. But our lab have just started getting our own nanopore data, so I'll test in that myself. It's only isolate data so shouldn't take too long

@jfy133
Copy link
Member

jfy133 commented May 28, 2025

@nf-core-bot fix linting

@jfy133
Copy link
Member

jfy133 commented Jun 5, 2025

@muabnezor From my test - and now looking at test_long output here too, the final bin_summary process doesn't get executed.

Is that to be expected? This is one of the critical output files...

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.

Code-wise I'm happy now!

I just want to explore the results a bit more (see comment above about missing bin_summary.tsv process

Comment on lines +57 to +58
longread_percentidentity = null
shortread_percentidentity = null
Copy link
Member

Choose a reason for hiding this comment

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

@muabnezor is there any reason why you didn't set this?

@muabnezor
Copy link
Contributor Author

@muabnezor From my test - and now looking at test_long output here too, the final bin_summary process doesn't get executed.

Is that to be expected? This is one of the critical output files...

no, this is not expected, are any of the channels from your test empty going into the process?

@jfy133
Copy link
Member

jfy133 commented Jun 5, 2025

@muabnezor From my test - and now looking at test_long output here too, the final bin_summary process doesn't get executed.
Is that to be expected? This is one of the critical output files...

no, this is not expected, are any of the channels from your test empty going into the process?

I'm running another long running test on my laptop at the moment so I can't check (unfortuantely)... any chance you could look? Otherwise I will do tomorrow

@muabnezor
Copy link
Contributor Author

I'm running another long running test on my laptop at the moment so I can't check (unfortuantely)... any chance you could look? Otherwise I will do tomorrow

Sorry, can't get to that this week, I will have a look at it on monday

@muabnezor
Copy link
Contributor Author

@jfy133
I'm looking into the bin_summary stuff, it ran fine a couple of commits ago, and I missed that it stopped running. good catch. I don't know if it is something I've done, or if the DEPTH process has had any changes recently, since I haven't touched anything after all the binning is done in this PR I think. I will try to get to what's wrong, but I'm currently having a lot of work to be done before summer vacations, so I don't know how much time I can spend on this.

@jfy133
Copy link
Member

jfy133 commented Jun 6, 2025

I did some exploration last night: https://nfcore.slack.com/archives/CE9MS66BS/p1749154196789889?thread_ts=1748503770.520549&cid=CE9MS66BS

Essentially for some reason there is an extra meta being added somewhere meaning the reeads and bins can't be combined in DEPTHS()

@dialvarezs as you are waiting for this to be able to continue with your stuff, any chance you could have a look?

@jfy133
Copy link
Member

jfy133 commented Jun 6, 2025

And to clarify @muabnezor it's not the DEPTHS subworkflow itself that is the issue, just the contents of the input channels :)

@muabnezor
Copy link
Contributor Author

muabnezor commented Jun 6, 2025

@jfy133, there, I think I fixed it in the last commit in how depth channel is returned from binning workflow. I think (and hope!) this will fix it. Thanks for helping with this!

@jfy133
Copy link
Member

jfy133 commented Jun 6, 2025

@muabnezor testing!

@jfy133 jfy133 self-requested a review June 6, 2025 11:35
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.

Can confirm bin_summary.tsv now generated!

Everything else we can fix in post-production 😬

Thank you very very very much @muabnezor ! For the huge effort and your patience on this one, like I said before - many people are going to be very happy!

Hit that merge button!

@muabnezor muabnezor merged commit 56ca005 into nf-core:dev Jun 6, 2025
17 checks passed
@muabnezor
Copy link
Contributor Author

@jfy133 just merged! Thanks for all the help with this PR!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add separate Nanopore input option

7 participants