use a meta map by maxulysse · Pull Request #256 · nf-core/sarek

maxulysse · 2020-07-23T20:44:23Z

Using a meta map à la @drpatelh

nf-core/sarek pull request

Many thanks for contributing to nf-core/sarek!

Please fill in the appropriate checklist below (delete whatever is not relevant).
These are the most common things requested on pull requests (PRs).

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 necessary, also make a PR on the nf-core/sarek branch on the nf-core/test-datasets repo
Ensure the test suite passes (nextflow run . -profile test,docker).
Make sure your code lints (nf-core lint .).
Documentation in docs is updated
CHANGELOG.md is updated
README.md is updated

Learn more about contributing: CONTRIBUTING.md

drpatelh · 2020-07-23T20:58:14Z

Thanks @maxulysse! Will have a proper look at this tomorrow at some point 👍

FriederikeHanssen

Love how readable everything is now 😍

FriederikeHanssen · 2020-07-24T07:10:01Z

-    fastqc -t 2 -q ${idSample}_${idRun}_R1.fastq.gz ${idSample}_${idRun}_R2.fastq.gz
+    [ ! -f  ${prefix}_1.fastq.gz ] && ln -s ${reads[0]} ${prefix}_1.fastq.gz
+    [ ! -f  ${prefix}_2.fastq.gz ] && ln -s ${reads[1]} ${prefix}_2.fastq.gz
+    fastqc --threads ${task.cpus} ${prefix}_1.fastq.gz ${prefix}_2.fastq.gz


Out of curiosity: What does [! -f ...] do?

No idea, just copied it over from chipseq.
I figured it would be easier to update from the nf-core modules from there

maybe @drpatelh then knows 😃

Its bash notation for checking if a file doesnt exist 🙂

FriederikeHanssen · 2020-07-24T07:14:57Z

+        ${extra} \
+        -t ${task.cpus} \
+        ${fasta} ${reads} | \
+    samtools sort --threads ${task.cpus} -m 2G - > ${meta.id}.bam


Why is the memory hardcoded here?

Did not paid attention enough there, I guess I just copied it over from the current sarek dev, I can set it back the way it was

drpatelh

Mainly some reorganisation of the module structure and consistency with using module options and syntax for reusability and flexibility.

drpatelh · 2020-07-24T08:31:21Z

+    def bwamem2_mem_options = [:]
+
+    bwamem2_mem_options.args_bwamem2 = "-K 100000000 -M"


These options can be pre-defined in a map in conf/modules.config like here, included via nextflow.config like here and then you can even append parameters like here in the main script if required.

Hopefully, this means the software parameters are easier to pass around the script and are more customisable by the developed/user. Also, means you dont have to initialise maps all over the place for the module settings because this is already explicitly done in modules.config.

We should stick the same notation to access variable in the module files though i.e. the 5 I have needed so far are listed here

This also means that every module will need to be passed an additional opts value which is a map containing the generic options for the module e.g. here because this will allow more flexibility to control things like output directory etc. like here

drpatelh · 2020-07-24T08:38:27Z

+    label 'process_high'

-    publishDir "${params.outdir}/bwamem2_mem", mode: 'copy'
+    publishDir "${params.outdir}/bwamem2/${meta.sample}",


You should remove any customisation from this code in terms of output directories as this should be customisable from the opts map that comes into the module. The logic still needs a little work but for now this is the generic code I am using here

drpatelh · 2020-07-24T08:41:15Z

    script:
    CN = params.sequencing_center ? "CN:${params.sequencing_center}\\t" : ""
-    readGroup = "@RG\\tID:${run}\\t${CN}PU:${run}\\tSM:${sample}\\tLB:${sample}\\tPL:${params.sequencer}"
+    readGroup = "@RG\\tID:${meta.run}\\t${CN}PU:${meta.run}\\tSM:${meta.sample}\\tLB:${meta.sample}\\tPL:ILLUMINA"


This should come in via the meta parameter because not everyone will want to create the read group in this way because they wont have all of the same values in map e.g. see here

drpatelh · 2020-07-24T08:41:29Z

+        tuple val(meta), path("*.bam"), path("*.bai")

    script:
    CN = params.sequencing_center ? "CN:${params.sequencing_center}\\t" : ""


Suggested change

CN = params.sequencing_center ? "CN:${params.sequencing_center}\\t" : ""

This parameter should come in via arguments created from the pipeline and not hardcoded here. Not everyone will use this!

drpatelh · 2020-07-24T08:44:12Z

-    samtools sort --threads ${task.cpus} -m 2G - > ${sample}_${run}.bam
-    samtools index ${sample}_${run}.bam
+    bwa-mem2 mem \
+        ${options.args_bwamem2} \


Suggested change

${options.args_bwamem2} \

$opts.args \

drpatelh · 2020-07-24T08:45:40Z

+        ${fasta} ${reads} | \
+    samtools sort --threads ${task.cpus} -m 2G - > ${meta.id}.bam
+
+    samtools index ${meta.id}.bam


Will need to allow for suffixes too in order to allow for naming the bam files differently if required. This would apply to other modules too e.g. here

drpatelh · 2020-07-24T08:46:38Z

-    fastqc -t 2 -q ${idSample}_${idRun}_R1.fastq.gz ${idSample}_${idRun}_R2.fastq.gz
+    [ ! -f  ${prefix}_1.fastq.gz ] && ln -s ${reads[0]} ${prefix}_1.fastq.gz
+    [ ! -f  ${prefix}_2.fastq.gz ] && ln -s ${reads[1]} ${prefix}_2.fastq.gz
+    fastqc --threads ${task.cpus} ${prefix}_1.fastq.gz ${prefix}_2.fastq.gz


Its bash notation for checking if a file doesnt exist 🙂

maxulysse · 2020-07-24T08:50:52Z

for BWA_MEM2_MEM could we use BWA-MEM2_MEM instead actually?
I think it makes more sense

drpatelh · 2020-07-24T08:55:50Z

I am not too keen on using a mixture of _ and -...I think we should take this opportunity to unify the syntax as much as possible otherwise its the Wild West out there in terms of naming conventions! Also means it will easier to write downstream tools based on this structure if the syntax is the same!

maxulysse · 2020-07-24T09:04:27Z

name of the tool is now BWA-MEM2, so I'm all for BWAMEM2 or BWA-MEM2, but not keen on BWA_MEM2, I think it's confusing with the suffix we add for _INDEX or _MEM afterwards

drpatelh · 2020-07-24T09:41:51Z

Ah, I see. Good point 👍 If we want to split on the _ to be able to extract the sub-tool used too then it makes sense to maybe rethink this. Although, what do we do for example with trim_galore that is invoked as trim_galore - maybe that should be one word instead? 🤔

maxulysse · 2020-07-24T09:43:44Z

that's a good point

maxulysse added 10 commits July 23, 2020 22:15

use meta map

5e288ca

define meta map for extract_fastq()

1672006

use meta map

6738fbb

use meta map

a55315f

update module

a65be18

code polish

71f32fd

update version

5229f73

fix output

9dce2a0

use meta map

aa224dc

use meta map

9ea67af

maxulysse requested a review from FriederikeHanssen July 23, 2020 20:44

maxulysse mentioned this pull request Jul 23, 2020

Checking the sample sheet in DSL2 #235

Closed

maxulysse requested a review from drpatelh July 23, 2020 20:45

FriederikeHanssen approved these changes Jul 24, 2020

View reviewed changes

FriederikeHanssen merged commit 538a296 into nf-core:dsl2 Jul 24, 2020

drpatelh reviewed Jul 24, 2020

View reviewed changes

maxulysse deleted the dsl2_meta branch July 24, 2020 08:50

maxulysse added the DSL2 label Jan 26, 2021

		def bwamem2_mem_options = [:]

		bwamem2_mem_options.args_bwamem2 = "-K 100000000 -M"

Conversation

maxulysse commented Jul 23, 2020

nf-core/sarek pull request

PR checklist

Uh oh!

drpatelh commented Jul 23, 2020

Uh oh!

FriederikeHanssen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

drpatelh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxulysse commented Jul 24, 2020

Uh oh!

drpatelh commented Jul 24, 2020

Uh oh!

maxulysse commented Jul 24, 2020

Uh oh!

drpatelh commented Jul 24, 2020

Uh oh!

maxulysse commented Jul 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants