Added optional Bwa index parameter to skip bwamem2 index for a faster output #132
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Maxime U Garcia <[email protected]>
Co-authored-by: Maxime U Garcia <[email protected]>
…nspector into picard_tools_dev
Member
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 3.2.0. For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation. |
|
maxulysse
reviewed
Nov 7, 2025
Member
There was a problem hiding this comment.
Why a separate file for this, and not using the conf/base.config?
maxulysse
reviewed
Nov 7, 2025
Open
maxulysse
reviewed
Nov 7, 2025
Comment on lines
+26
to
+27
| - [#127] (https://github.com/nf-core/seqinspector/pull/127) Added alignment tools - bwamem2 - index and mem | ||
| - [#128] (https://github.com/nf-core/seqinspector/pull/128) Added Picard tools - Collect Multiple Mterics to collect QC metrics |
Member
There was a problem hiding this comment.
where is 132?
Suggested change
| - [#127] (https://github.com/nf-core/seqinspector/pull/127) Added alignment tools - bwamem2 - index and mem | |
| - [#128] (https://github.com/nf-core/seqinspector/pull/128) Added Picard tools - Collect Multiple Mterics to collect QC metrics | |
| - [#127](https://github.com/nf-core/seqinspector/pull/127) Added alignment tools - bwamem2 - index and mem | |
| - [#128](https://github.com/nf-core/seqinspector/pull/128) Added Picard tools - Collect Multiple Mterics to collect QC metrics |
maxulysse
reviewed
Nov 7, 2025
Member
There was a problem hiding this comment.
so for me bwa and other indexes should be fetch using getGenomeAttribute at this level from the igenomes.config file around L17
maxulysse
reviewed
Nov 7, 2025
| igenomes_base = 's3://ngi-igenomes/igenomes/' | ||
| igenomes_ignore = false | ||
| sort_bam = true | ||
| bwa_index = null |
maxulysse
reviewed
Nov 7, 2025
Comment on lines
+287
to
+291
| "properties": { | ||
| "bwa_index": { | ||
| "type": "string" | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
that should be in the reference_genome_options part
maxulysse
reviewed
Nov 7, 2025
Comment on lines
+14
to
+20
| picard_collectmultiplemetrics/*.pdf | ||
| picard_collectmultiplemetrics/*_metrics | ||
| picard_collectmultiplemetrics/alignment_summary_metrics | ||
| picard_collectmultiplemetrics/base_distribution_by_cycle_metrics | ||
| picard_collectmultiplemetrics/quality_by_cycle_metrics | ||
| picard_collectmultiplemetrics/quality_distribution_metrics | ||
| picard_collectmultiplemetrics/*_summary_metrics |
Member
There was a problem hiding this comment.
I feel this is coming from another PR
maxulysse
reviewed
Nov 7, 2025
| )) | ||
| ch_fastqscreen_refs = channel.fromList( | ||
| samplesheetToList( | ||
| params.fastq_screen_references, |
Member
There was a problem hiding this comment.
So for me this should not be done at this level, but in the root main.nf.
And no params should be used as is at this level, but coming from the root main.nf too.
maxulysse
reviewed
Nov 7, 2025
| } | ||
| else { | ||
| // Build index from reference FASTA when no pre-built index is provided | ||
| BWAMEM2_INDEX( |
Member
There was a problem hiding this comment.
I have a feeling this should be done in a separate prepare genome subworkflows prior to this actual workflow, happy to talk with you about some refactoring
maxulysse
reviewed
Nov 7, 2025
Comment on lines
+177
to
+181
| SAMTOOLS_FAIDX( | ||
| ch_reference_fasta, | ||
| ch_dummy_fai, | ||
| true, | ||
| ) |
Member
There was a problem hiding this comment.
I have a feeling this should be done in a separate prepare genome subworkflows prior to this actual workflow, happy to talk with you about some refactoring
maxulysse
reviewed
Nov 7, 2025
Member
maxulysse
left a comment
maxulysse
left a comment
There was a problem hiding this comment.
Nice work.
I feel like there is a bit of leftover from another PR regarding some picard tools, but that's really minor.
For me the code work as it should and that's good.
I do think all the references preparation should stripped out of the worklfow and moved out to a separate one.
But I think it's some refactoring that could be done separately.
Params should only be used in the root main.nf script, otherwise Nextflow will start complaining in future version very soon, but that can be done separately as well
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR checklist
nf-core lint).nf-test test main.nf.test -profile test,docker).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).