Add sans-UMI option #128
Merged
Add sans-UMI option #128
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
Release 1.2.0
Release 2.0.0
…tered reads (but before collapsing duplicates)
…ultiple files to output_dir instead of only one when multiple sources exist
…hold to 0 and print warning
ggabernet
reviewed
Aug 3, 2021
There was a problem hiding this comment.
Hi @dladd thanks for this PR, it has some great additions to the pipeline!
Seeing the number of changes required to add the sans-umi option, I've tried to make such addition a bit easier adding a presto_UMI sub-workflow (and maybe a bit easier to follow in the code for other people). Check out the #130 PR. Let me know what you think there, if that would make it easier to incorporate your changes with less if/else, also providing a subworkflow presto_sans_UMI (or sth similar), and maybe also for other people to add further pRESTO pre-processing subworkflows.
Co-authored-by: Gisela Gabernet <[email protected]>
Co-authored-by: Gisela Gabernet <[email protected]>
Co-authored-by: Gisela Gabernet <[email protected]>
Co-authored-by: Gisela Gabernet <[email protected]>
…res in user-defined sample names, remove unused tools in no-umi mode
ggabernet
reviewed
Aug 11, 2021
There was a problem hiding this comment.
Hi @dladd ,
it's looking great, thanks for the sans-umi addition! I just have a couple of comments, but otherwise looks good.
So the nf-core CI tests also pass, one can add the - assets/multiqc_config.yaml to the list of files that need to be ignored for the linting test, as you added a FastQC module description. It should be below this line:
workflows/bcellmagic.nf
Outdated
| @@ -46,7 +46,7 @@ if (params.protocol == "pcr_umi"){ | |||
|
|
|||
| // Validate UMI position | |||
| if (params.index_file & params.umi_position == 'R2') {exit 1, "Please do not set `--umi_position` option if index file with UMIs is provided."} | |||
| if (params.umi_length == 0) {exit 1, "Please provide the UMI barcode length in the option `--umi_length`."} | |||
| if (params.umi_length == null) {exit 1, "Please provide the UMI barcode length in the option `--umi_length`. To run without UMIs, set umi_length to 0."} | |||
There was a problem hiding this comment.
Suggested change
| if (params.umi_length == null) {exit 1, "Please provide the UMI barcode length in the option `--umi_length`. To run without UMIs, set umi_length to 0."} | |
| if (!params.umi_length) {exit 1, "Please provide the UMI barcode length in the option `--umi_length`. To run without UMIs, set umi_length to 0."} |
There was a problem hiding this comment.
I've swapped this back to check for params.umi_length == null. Apparently in groovy !0 == true so running a no-UMI pipeline would otherwise always trigger the exit condition
There was a problem hiding this comment.
Hmm it doesn't look like the linting likes us defining an integer param with null as the default in the schema. I've set default umi_length = -1 as a workaround but let me know if you have a different preferred solution.
There was a problem hiding this comment.
ok weird, I've tested it in the nextflow console (nextflow console) and it was interpreted as false:
def foo = null
var= "hello"
if (!foo) {
print var
}
But yes you are right, the schema does not like an integer defined by default as null. The proposed -1 solution sounds good to me!
There was a problem hiding this comment.
The issue was that with no-UMI, we are defining foo = 0, which is interpreted the same as foo = null and triggers the exit. Cool will stick with the -1 default workaround :-)
conf/modules.config
Outdated
| 'presto_parseheaders_collapse_sans_umi' { | ||
| publish_dir = false | ||
| subcommand = 'collapse' | ||
| args = '-f CONSCOUNT --act min' |
There was a problem hiding this comment.
I'm not sure about this here, as far as I understand it the CONSCOUNT value is the count of sequences having the same UMI barcode use to build a consensus. So without UMIs you should actually not have this field.
There was a problem hiding this comment.
Good catch! This was a vestigial step that snuck under my radar. Removed in d8dc9d0
Co-authored-by: Gisela Gabernet <[email protected]>
Co-authored-by: Gisela Gabernet <[email protected]>
Co-authored-by: Gisela Gabernet <[email protected]>
Co-authored-by: Gisela Gabernet <[email protected]>
Co-authored-by: Gisela Gabernet <[email protected]>
ggabernet
reviewed
Aug 12, 2021
Co-authored-by: Gisela Gabernet <[email protected]>
ggabernet
reviewed
Aug 13, 2021
|
…ated test bug. The umi_length > 0 check in the pipeline and -1 default should cover this; update changelog
ggabernet
approved these changes
Aug 26, 2021
Co-authored-by: Gisela Gabernet <[email protected]>
Closed
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.
Addresses issue #127
PR checklist
params.umi_length = 0. Will be-1by default to remind users to set this value when running the pipelineprimer_revprnf-core lint .).nextflow run . -profile test,docker).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).