Shortread subworkflow #716

muabnezor · 2024-11-23T06:39:39Z

PR checklist

This PR moves the short read preprocessing into a subworkflow

github-actions · 2024-11-23T06:39:50Z

This PR is against the `master` branch ❌

Do not close this PR
Click Edit and change the base to dev
This CI test will remain failed until you push a new commit

It looks like this pull-request is has been made against the muabnezor/mag master branch.
The master branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master are only allowed if they come from the muabnezor/mag dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

…s now handled in subworkflow

muabnezor · 2024-11-24T20:02:10Z

Think this is ready for review @jfy133. Simple PR moving short read preprocessing into subworkflow.

…to always declare the ch_short_reads_assembly even if assembly-input

jfy133

Looks like a straight forward conversion to me! thank you @muabnezor you're making the code base much cleaner!

jfy133 · 2024-11-25T07:39:51Z

subworkflows/local/shortread_preprocessing.nf

+include { FASTQC as FASTQC_RAW                                  } from '../../modules/nf-core/fastqc/main'
+include { FASTQC as FASTQC_TRIMMED                              } from '../../modules/nf-core/fastqc/main'
+include { FASTP                                                } from '../../modules/nf-core/fastp/main'
+include { ADAPTERREMOVAL as ADAPTERREMOVAL_PE                   } from '../../modules/nf-core/adapterremoval/main'
+include { ADAPTERREMOVAL as ADAPTERREMOVAL_SE                   } from '../../modules/nf-core/adapterremoval/main'
+include { BOWTIE2_REMOVAL_BUILD as BOWTIE2_HOST_REMOVAL_BUILD   } from '../../modules/local/bowtie2_removal_build'
+include { BOWTIE2_REMOVAL_ALIGN as BOWTIE2_HOST_REMOVAL_ALIGN   } from '../../modules/local/bowtie2_removal_align'
+include { BOWTIE2_REMOVAL_BUILD as BOWTIE2_PHIX_REMOVAL_BUILD   } from '../../modules/local/bowtie2_removal_build'
+include { BOWTIE2_REMOVAL_ALIGN as BOWTIE2_PHIX_REMOVAL_ALIGN   } from '../../modules/local/bowtie2_removal_align'
+include { CAT_FASTQ                                            } from '../../modules/nf-core/cat/fastq/main'
+include { SEQTK_MERGEPE                                        } from '../../modules/nf-core/seqtk/mergepe/main'
+include { BBMAP_BBNORM                                         } from '../..//modules/nf-core/bbmap/bbnorm/main'


Please align :)

If you use VSCode, you should install the Nextflow extension which now includes a formatter you can activate via the command palette and should do that for you :)

Will do. thanks for suggestion.

muabnezor · 2024-11-25T07:44:25Z

subworkflows/local/shortread_preprocessing.nf

+    ch_multiqc_files = ch_multiqc_files.mix(FASTQC_RAW.out.zip)
+
+    ch_bowtie2_removal_host_multiqc = Channel.empty()
+    if (!params.assembly_input) {


if (!params.assembly_input) in the future, it feels like we can move away from this. Could the assembly_input logic be determined by the users input?

I was wondering about this actually myself:

If you've pre-assembled reads, they should already be cleaned. Which means you could skip this entire subworkflow, so maybe this if else statement could be pushed to the main mag.nf around the new subworkflow? @prototaxites what do you think, is that a reasonable assuemption to make (i.e., skip preprocessing of reads)

But what do you mean by 'users input'? This is basically that, if you give the additional extra sheet to --assembly_input (that contains the paths to contig FASTAS) then we can skip assembly. It's not a boolean flag, if that is what you were meaning.

But what do you mean by 'users input'? This is basically that, if you give the additional extra sheet to --assembly_input (that contains the paths to contig FASTAS) then we can skip assembly. It's not a boolean flag, if that is what you were meaning.

Ah, then this makes more sense. I thought it was a boolean.

If you've pre-assembled reads, they should already be cleaned. Which means you could skip this entire subworkflow, so maybe this if else statement could be pushed to the main mag.nf around the new subworkflow?

yes, I actually did this first, but then moved it back into the subworkflow. Not sure what makes the most sense.

If you've pre-assembled reads, they should already be cleaned. Which means you could skip this entire subworkflow, so maybe this if else statement could be pushed to the main mag.nf around the new subworkflow? @prototaxites what do you think, is that a reasonable assuemption to make (i.e., skip preprocessing of reads)

Yes, I think this probably makes the most sense. While I'm sure there's someone out there with the use-case that they want to map a new set of reads against an existing assembly? I think we should assume that the reads accompanying pre-computed assemblies are already clean - someone with that use case probably knows what they are doing!

Personally I'm in favour of putting the if!(assembly_input) line in the main WF rather than the subWF; the subWF code should only operate when we actually want to use it. Plus I think it helps make the logic of the main WF clearer by stating when and when not things are run.

Thanks for input. I think it makes sense to have it in the main workflow also. If you think this looks fine @jfy133 and @prototaxites I'll go ahead and merge.

jfy133 · 2024-11-25T10:09:13Z

subworkflows/local/shortread_preprocessing.nf

+    ch_versions = ch_versions.mix(FASTQC_RAW.out.versions.first())
+    ch_multiqc_files = ch_multiqc_files.mix(FASTQC_RAW.out.zip)
+
+    ch_bowtie2_removal_host_multiqc = Channel.empty()


Not sure why we have a dedicated channel for that... maybe can be mixed directly into ch_multiqc_files

Doesn't even seem to be used anyway so can be removed :)

jfy133 · 2024-11-25T10:09:56Z

subworkflows/local/shortread_preprocessing.nf

+    ch_versions = ch_versions.mix(FASTQC_RAW.out.versions.first())
+    ch_multiqc_files = ch_multiqc_files.mix(FASTQC_RAW.out.zip)
+
+    ch_bowtie2_removal_host_multiqc = Channel.empty()


Suggested change

ch_bowtie2_removal_host_multiqc = Channel.empty()

jfy133 · 2024-11-25T10:11:27Z

workflows/mag.nf

-            def new_reads = meta.single_end ? reads[0] : reads.flatten()
-            [meta, new_reads]
-        }
+        SHORTREAD_PREPROCESSING(


I think this stil needs the !params.assembly_input bit? And the else should then be assigning the raw reads directly to ch_short_reads and ch_short_reads_assembly etc (assuming it has the right channel construct)

It is there, line 191, and 205?

muabnezor added 3 commits November 1, 2024 15:03

move short read preprocessing into a subworkflow

4552dc7

add include statements

00ac925

Merge remote-tracking branch 'upstream/dev' into shortread_subworkflow

65c8cea

muabnezor added the WIP Work in progress label Nov 23, 2024

muabnezor requested a review from jfy133 November 23, 2024 06:39

muabnezor changed the base branch from master to dev November 23, 2024 06:40

muabnezor added 4 commits November 23, 2024 10:09

Fix multiqc input channel, channel mixing for short read processing i…

2768f5e

…s now handled in subworkflow

Fix multiqc file mix

9a039da

Move assembly-input logic to main workflow

7861527

Update Changelog

745a6c1

muabnezor removed the WIP Work in progress label Nov 24, 2024

muabnezor added 2 commits November 24, 2024 21:04

remove include statements from main workflow

9273717

Move assembly-input logic back into shortread subworkflow. Make sure …

16ba808

…to always declare the ch_short_reads_assembly even if assembly-input

jfy133 approved these changes Nov 25, 2024

View reviewed changes

muabnezor commented Nov 25, 2024

View reviewed changes

muabnezor added 2 commits November 25, 2024 09:27

Format include statements

f2507b2

Move assembly-input logic back to main workflow

0b3c9ba

jfy133 reviewed Nov 28, 2024

View reviewed changes

Remove unused bowtie2 multiqc channel

2262d60

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shortread subworkflow #716

Shortread subworkflow #716

muabnezor commented Nov 23, 2024 •

edited

Loading

github-actions bot commented Nov 23, 2024

muabnezor commented Nov 24, 2024

jfy133 left a comment

jfy133 Nov 25, 2024

muabnezor Nov 25, 2024

muabnezor Nov 25, 2024

jfy133 Nov 25, 2024

muabnezor Nov 25, 2024

muabnezor Nov 25, 2024

prototaxites Nov 25, 2024

muabnezor Nov 25, 2024

jfy133 Nov 25, 2024

jfy133 Nov 25, 2024

jfy133 Nov 25, 2024

jfy133 Nov 25, 2024

muabnezor Nov 29, 2024

Shortread subworkflow #716

Are you sure you want to change the base?

Shortread subworkflow #716

Conversation

muabnezor commented Nov 23, 2024 • edited Loading

PR checklist

github-actions bot commented Nov 23, 2024

This PR is against the master branch ❌

muabnezor commented Nov 24, 2024

jfy133 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muabnezor commented Nov 23, 2024 •

edited

Loading

This PR is against the `master` branch ❌