Add subsampling #50
Merged
Add subsampling #50
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
|
MatthiasZepper
requested changes
Oct 28, 2024
There was a problem hiding this comment.
Thanks a lot for your contribution! The subsampling functionality will be very important to Seqinspector, therefore it is great that you decided to work along this line!
Unfortunately, there is still some room for improvement, but I am optimistic that all of those issues can be resolved during the next two days of the Hackathon.
Generally, I think, that most users would want to select a fraction of reads to subsample rather than deciding for an absolute number of reads. Therefore, your initial idea of using SeqKit sample, which has direct support for probabilities, would make a lot of sense, however, require writing the module first. If going with Seqtk, I think it would be fine merging this PR with absolute numbers only, but if you feel enthusiastic, you could patch the module to include a read counting step and set the sample size according to the number of input reads.
| script: | ||
| def args = task.ext.args ?: '' | ||
| def prefix = task.ext.prefix ?: "${meta.id}" | ||
| if (!(args ==~ /.*-s[0-9]+.*/)) { |
There was a problem hiding this comment.
Do you think it would be useful to expose the random seed of the tool as (possibly hidden) pipeline parameter?
There was a problem hiding this comment.
I'm not sure about this, is there any example you have in mind?
There was a problem hiding this comment.
I am unsure as well. My experience from the RNA-seq pipeline is, that people usually struggle with custom configuration files.
The extra_* convenience parameters of the pipeline (extra_star_align_args or extra_ kallistro_quant_args), which allow modifying the arguments of those tools without a custom config are very popular.
Therefore, I about setting the seed via a parameter, but on a second thought, I agree that the need for a custom config is probably fine.
There was a problem hiding this comment.
I can understand the difficulties with custom configuration files, the extra arguments are ok, but on the other side I have the impression that might be easily forgotten when setting the workflow. I think may be better to check if there are paired reads by checking the input channel or just if there is _1 and _2 files, and if present, enables the -s option?
workflows/seqinspector.nf
Outdated
| // MODULE: Run Seqkit sample to perform subsampling | ||
| // | ||
| if (params.sample_size > 0 ) { | ||
| ch_sample_sized = SEQTK_SAMPLE(ch_samplesheet.map { |
There was a problem hiding this comment.
Unfortunately, I don't think the module is capable of processing paired files. The description of the tool suggests that it has to be invoked with the same random seed on each FastQ file separately:
Subsample 10000 read pairs from two large paired FASTQ files (remember to use the same random seed to keep pairing):
seqtk sample -s100 read1.fq 10000 > sub1.fq
seqtk sample -s100 read2.fq 10000 > sub2.fq
and all of the module tests only use a single file.
When you invoke the module, you will have to account for that.
There was a problem hiding this comment.
Ok, you know if at least one of the tests have paired reads so I can view how the input channel looks like and also test this functionality?
I have found this post about custom-configuration-files and looking the module, the '-s' option is available using it as ext.args.
There was a problem hiding this comment.
I have asked in Barcelona, and apparently there isn't yet a paired-end test, so I has been suggested to open an issue to add it: #55
For now I have added the seed argument in the conf/modules.config
nggvs
commented
Oct 29, 2024
workflows/seqinspector.nf
Outdated
| // MODULE: Run Seqkit sample to perform subsampling | ||
| // | ||
| if (params.sample_size > 0 ) { | ||
| ch_sample_sized = SEQTK_SAMPLE(ch_samplesheet.map { |
There was a problem hiding this comment.
Ok, you know if at least one of the tests have paired reads so I can view how the input channel looks like and also test this functionality?
I have found this post about custom-configuration-files and looking the module, the '-s' option is available using it as ext.args.
MatthiasZepper
approved these changes
Oct 29, 2024
| @@ -18,6 +18,10 @@ process { | |||
| saveAs: { filename -> filename.equals('versions.yml') ? null : filename } | |||
| ] | |||
|
|
|||
| withName: SEQTK_SAMPLE { | |||
| ext.args = '-s100' | |||
| } | |||
There was a problem hiding this comment.
I have no opinion on whether the pipeline should publish the subsampled files. But if you want to output them in the way you describe in the output.md, you will probably need to define a corresponding publishDir directive here? (There is also a new possibility called Workflow Output Schema, that I am however not yet familiar with. Did you use that without me noticing it?)
There was a problem hiding this comment.
I think is always output because there is a general statement here:
seqinspector/conf/modules.config
Line 15 in a2d589f
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.
Closes #28.
Default 0 means that no subsampling is performed.
The subsampling step is performed at the very beginning of the pipeline.
Regarding this:
SeqKit sample already uses a default according to the documentation:
https://bioinf.shenwei.me/seqkit/usage/#sample see
-sSnapshots updated after merging #49
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).