Added ability to auto-create samplesheet for nf-core/phageannotator #543
Conversation
This file contains 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
|
jfy133
reviewed
Nov 21, 2023
There was a problem hiding this comment.
Ran out of time before baby came back so couldn't test, but first pass: very clean code thank you very much @CarsonJM !
Couple of things missing (docs atm):
- Missing changelog update
- Missing update to README
- Missing input/output docs
jfy133
reviewed
Dec 14, 2023
|
@nf-core-bot fix linting |
jfy133
reviewed
Feb 9, 2024
There was a problem hiding this comment.
Non-blocking thoughts:
- Q: I wonder if we should put this into an umbrella workflow rather than the if/else statement
- Q: The
testprofile should ideally betest_generatesamplesheetrather than justtest_samplesheet(someone might think it's to use an input samplesheet) - Q: Are you definitely sure people would always want to collapse all assemblies per sample (no cases they would want to keep it separate)
Co-authored-by: James A. Fellows Yates <[email protected]>
|
Regarding your questions:
|
jfy133
reviewed
Mar 15, 2024
CarsonJM
commented
Mar 17, 2024
Co-authored-by: James A. Fellows Yates <[email protected]>
CarsonJM
commented
Mar 17, 2024
Comment on lines
106
to
112
| // Create samplesheet for each sample using meta information | ||
| ch_mag_id_samplesheets = ch_mag_metadata.collectFile() { meta -> | ||
| [ "${meta.id}_phageannotator_samplesheet.csv", "sample,group,fastq_1,fastq_2,fasta" + '\n' + "${meta.id},${meta.group},${meta.fastq_1},${meta.fastq_2},${meta.fasta}" + '\n' ] | ||
| } | ||
|
|
||
| // Merge samplesheet across all samples for the pipeline | ||
| ch_mag_id_samplesheets.collectFile(name: "phageannotator_samplesheet.csv", keepHeader:true, skip:1, storeDir:"${params.outdir}/downstream_samplesheets/") |
There was a problem hiding this comment.
@jfy133 Just to clarify, are you thinking I should add the storeDir:"${params.outdir}/downstream_samplsheets/" to the first collectFile()?
There was a problem hiding this comment.
No, more don't bind just fasta to the string on the line above.
params.outdir + <path I don't remember> + fasta.name or something
There was a problem hiding this comment.
For the FastQ files, it's possible that the users don't specify to save them to outdir, so should part of the generate_downstream_samplesheet be to copy the files to outdir even if not specified elsewhere?
There was a problem hiding this comment.
Or even just validate that if you turn on samplesheet creation, then you must also save the fastqs.
But yes, one way or another - running from explicitly saved fsstqs in outdir is preferable.
If you have cleanup = true in a config, something many do but the user never sees, they won't have any fastq files in the work dir to point to in the sample sheet 😬. This is harder to control for rather than directing the user to make sure to save the fsstqs
There was a problem hiding this comment.
Makes sense to me!
jfy133
reviewed
Mar 22, 2024
Comment on lines
+110
to
+111
| file(meta.fastq_1.toUriString(), checkIfExists: true).copyTo("${params.outdir}/downstream_samplesheets/fastq/${meta.fastq_1.name}") | ||
| file(meta.fasta, checkIfExists: true).copyTo("${params.outdir}/downstream_samplesheets/fasta/${meta.fasta.name}") |
There was a problem hiding this comment.
Asking for some advise about this, this is indeed very clean, but given it's outside a process/publishdir I'm a bit unsure. E.g. some people use symlinks from work to results dir rather than copy, so this would violate this.
Secondly this woudl potentially result in two copies of the same read files if one of the --save_* parameters are given.
It maybe we have to come up with some complicated logic that instead picks the right 'final' directory for the proceessing reads, and it's that that you append to the beginning, and then add.
I think this would come with two steps:
-
Make sure one of
--save_clipped_reads,--save_hostremoved_reads,--save_phixremoved_reads,--save_bbnorm_readsare selected ifparams.generate_samplesheetis trueSo some input validation code like:
if ( params.generate_samplesheet && ![params.saved_clipped, save.hostremoved <...>].any() ) { Nextflow.error('[nf-core/mag] ERROR: must at least save one XYX if --generate_samplesheet <...>') } -
Then have to have some complicated logic to select which directory gets appended to the beginning of the file name (probably best in a case when but w/e), e.g.
if (params.save_clipped_reads && !params.save_phix && !params.save_host_removed && !params._save_bbnorm) {
samplehseet_reads_dir = /subdir/clipedreads/saved/in
} else if ( params.save_phix && !params.save_host_removed && !params._save_bbnorm){
samplesheet_reads_dir = /subdir/phixreads/saved/in
}
```
There was a problem hiding this comment.
Totally see what you're saying and was curious what thoughts would be about doing this outside a process. I am more than happy change it to what you're suggesting!
The reason I was giving this approach a try was I was hoping to find a generalizable approach that would work across pipelines/updates even if the save input logic you're talking about gets modified (also, I selfishly wanted to find a quicker approach haha) Also, I was hoping to force a copy because in the past I've symlinked to publishDir thinking I copied and had my reads disappear because my workDir is in a scratch directory 😅.
Still, I definitely have though about what you're saying and will make that change if you think it's best!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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).nextflow run . -profile 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).