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.

Sign up for GitHub

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

Jump to bottom

Revamping Dragonflye: Introducing Hybrid Mode & Patch Updates #106

Merged
merged 11 commits into from
Nov 9, 2023
Merged

Revamping Dragonflye: Introducing Hybrid Mode & Patch Updates #106

merged 11 commits into from
Nov 9, 2023

Conversation

Daniel-VM
Copy link
Contributor

@Daniel-VM Daniel-VM commented Nov 7, 2023 •
edited
Loading

PR DESCRIPTION

Enhancements:

  • New hybrid mode: allow dragonflye to polish draft genome with short-reads. (Closes Allow dragonflye to polish draft genome with short-reads  #105)
  • Version update of the nf-core/dragonglye (patch)
  • Fix modules.config (dragonflye section). The "$" in the 'if condition' was causing unexpected output.
  • Rename output file of nf-core/porechop (patch) to avoid overwritting the raw-input file .

RELEVANT NOTES

I have tried to test the dragonflye "hybrid-mode" with the conf/test_hybrid.conf profile. However, the assembly couldn't be resolved (it ended in 0 contigs)...

nextflow run main.nf \
        -profile singularity,test_hybrid \
        --assembler 'dragonflye' \
        --outdir ./results

I locally ran several test with other samples and it successfully assemble the genomes.

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/bacass 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>).
  • 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).

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅

Posted for pipeline commit 544fbbb

+| ✅ 159 tests passed       |+
#| ❔   1 tests were ignored |#

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.10
  • Run at 2023-11-09 10:08:45

d4straub
d4straub reviewed Nov 7, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have tried to test the dragonflye "hybrid-mode" with the conf/test_hybrid.conf profile. However, the assembly couldn't be resolved (it ended in 0 contigs)...

I find that not too surprising. That sample was set up for unicycler, i.e. sufficient coverage with short reads but very shallow nanopore reads, I assume. For dragonflye the setup needs to be different.
Probably easiest to add to the existing (?) long read assembly files some shallow short reads files?

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
modules/nf-core/porechop/porechop/main.nf Outdated Show resolved Hide resolved
modules/nf-core/dragonflye/main.nf Outdated Show resolved Hide resolved
@Daniel-VM Daniel-VM marked this pull request as ready for review November 7, 2023 10:17
@Daniel-VM
Copy link
Contributor Author

Daniel-VM commented Nov 7, 2023 •
edited
Loading

Probably easiest to add to the existing (?) long read assembly files some shallow short reads files?

Do the short-reads need to come from the same sample that was used for sequencing the longreads (samplename: subset15000.fastq.gz, samplesheet ) in the conf/test_long.config? (I assume that they have to, but just to confirm)

@d4straub
Copy link
Collaborator

d4straub commented Nov 7, 2023

Ideally yes, the same bacteria/strain/isolate would be sufficient too I think. The test just needs to succeed. Its just a test for functionality and not performance...

@Daniel-VM
Copy link
Contributor Author

Alright.

I cannot find where the sample subset15000.fastq.gz comes from nor where the associated short reads are. Is it possible to upload my own (downsampled) *.fastq.gz + samplesheet files to nf-core/test-dataset:bacass?.

@d4straub
Copy link
Collaborator

d4straub commented Nov 7, 2023

Sure, just make sure to not overwrite anything (older tests will fail then) and that the data you upload is small as possible but tests will pass.

@Daniel-VM
Copy link
Contributor Author

This last commit contains the suggested changes and a new test profile to test draognflye hybrid mode (nf-core/test-datasets#1040)

@d4straub
Copy link
Collaborator

d4straub commented Nov 9, 2023

Very nice! Just one problem: the draognflye hybrid test seems to be not executed here in the CI tests?
This can be easily activated by adding test_hybrid_dragonflye to

bacass/.github/workflows/ci.yml

Line 55 in 39de87c

profile: [test_long_miniasm, test_hybrid, test_long, test_dfast]
(and any other tests that might make sense, just NOT test_full because it would require too much resources).
Once that is done and the tests passes here then the PR is good to be merged I think.

@Daniel-VM
Copy link
Contributor Author

Yess! I forgot that one ;).

@Daniel-VM
Copy link
Contributor Author

Tests passed, I think that we can merge it ;).

d4straub
d4straub approved these changes Nov 9, 2023
View reviewed changes
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.

Great!

@Daniel-VM Daniel-VM merged commit 126e080 into nf-core:dev Nov 9, 2023
11 checks passed
@Daniel-VM Daniel-VM mentioned this pull request Mar 21, 2024
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d4straub d4straub d4straub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Daniel-VM @d4straub