-
Notifications
You must be signed in to change notification settings - Fork 3
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
Adding single-read functionality to PROFILE #84
Conversation
… back to run_dev_se.nf
…s, if clauses in the workflow)
…atement to original version.
…S3." This reverts commit 79f9222.
modules/local/bbduk/main.nf
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonleandergrimm any particular reason you're using BBDUK rather than BBDUK_HITS here? The main pipeline uses the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix these things, also your tests are failing, I think it has to do with the taxonomy workflow.
subworkflows/local/taxonomy/main.nf
Outdated
@@ -6,11 +6,17 @@ | |||
| MODULES AND SUBWORKFLOWS | | |||
***************************/ | |||
|
|||
include { BBMERGE } from "../../../modules/local/bbmerge" | |||
include { SUMMARIZE_BBMERGE } from "../../../modules/local/summarizeBBMerge" | |||
if (params.single_end) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonleandergrimm i chatted with Will, he said that it's okay to remove the if statement (we only previously had the if statement because nextflow would throw warnings, but that's been removed now). Please update this file to reflect this (i.e. remove the if statement).
subworkflows/local/hv_screen/main.nf
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be an empty file? Why is it here?
test-data/nextflow.config
Outdated
@@ -7,7 +7,9 @@ params { | |||
|
|||
// Directories | |||
base_dir = "s3://nao-mgs-wb/test-batch" // Parent for working and output directories (can be S3) | |||
ref_dir = "s3://nao-mgs-wb/index-20241113/output" // Reference/index directory (generated by index workflow) | |||
|
|||
ref_dir = "s3://nao-mgs-wb/index-20241113/output" // Reference/index directory (generated by index workflow) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the new index now
@simonleandergrimm @harmonbhasin This would be a good time to update the CHANGELOG. |
…dentified by Will
…rocess naming. removed hvscreen main.nf
…y/mgs-workflow into single-read-profile
@willbradshaw Made the requested changes, updated the changelog. I think this is good to go in. |
@harmonbhasin are you happy for this to get merged? I don't see anything blocking on my end. |
@willbradshaw feel free to merge, didn't realize my changes were still requested, apologies for that! |
FWIW, do we have clear norms regarding ownership handling of PRs? I think
some of them are languishing at times because both reviewee and reviewer
think they are in the other person's court (I think this happened to me and
Will sometimes).
Jeff and I use the "Assigned" feature to make it clear who owns a PR, with
the expectation of getting the PR back into the other person's hand within
one day (and assigning it back to them).
…On Mon, 23 Dec 2024 at 16:51, Harmon Bhasin ***@***.***> wrote:
@willbradshaw <https://github.com/willbradshaw> feel free to merge,
didn't realize my changes were still requested, apologies for that!
—
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AN7ASMRMDPCYU2YYOIUP5P32HAWOTAVCNFSM6AAAAABRL6K6POVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJZHEZTONJSGU>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
This PR adds support for single-read sequencing data to the PROFILE stage of the pipeline while maintaining existing paired-end functionality.
Key Changes
Testing
I validated the pipeline changes and compared single vs paired-end results in this notebook: https://data.securebio.org/simons-notebook/posts/2024-10-28-mgs-taxonomy-eval/
I find that single-read results look very similar to paired-end results. The paired-end results of run_dev_se.nf look the same as the results of run.nf, suggesting that the inclusion of single-read functionality doesn't negatively impact paired-end analysis.