Exposing Variant Filtering Params #104
Conversation
|
unclear what the functional distinction between For now, |
|
|
definitions/detect_variants.wdl
Outdated
| @@ -42,6 +42,8 @@ workflow detectVariants { | |||
| Float varscan_p_value = 0.99 | |||
| Float? varscan_max_normal_freq | |||
|
|
|||
| Float? min_var_freq = varscan_min_var_freq | |||
There was a problem hiding this comment.
This change I don't follow. Currently, if one does not specify any cutoffs in the input YAML. The min_var_freq that will get used in tools.fp_filter.wdl is 0.05. It seems like this would set that default to be 0.1. I would like these two to be independent and if one doesn't specify anything all the behavior should stay exactly as it is now.
There was a problem hiding this comment.
I think the way this PR is setup right now, if someone ran immuno.wdl then min_var_freq would get set to 0.05 by default. And if someone ran somatic_exome.wdl then min_var_freq would be set to 0.05 via varscan_min_var_freq. But if someone were to run detect_variants.wdl directly, then min_var_freq would be set to 0.1 via varscan_min_var_freq. I think this is confusing.
There was a problem hiding this comment.
I will assert that best practice should be to set the default value for a tool in the tool itself, and then only expose it upstream (or override it) when necessary because different users might want it set differently.
This seems like a case where it could be set as 0.05 on the tool, then exposed (with no default) upstream for folks that would like to change it. (I do know that people are modifying this one sometimes for low-VAF calling)
|
I think we should reconsider why we are setting defaults for these cutoffs at various places in the chain from immuno -> somatic -> detect variants -> variant caller sub workflow -> variant caller tool or false positive filter itself. I think we want the user running analysis from each of these points to be able override the default, but unless we are explicitly doing it on purpose for a good reason, why should we define a default value anywhere other than the final place it is about to be used (in this case either in varscan or the false positive filter)? |
|
Yeah, agree with Malachi. This seems like an opportunity to clean up some of this parameter passing//logic (see my comment inline upthread). It's gotten that way over time, through changes by lots of different folks, but we can draw a line in the sand and say "the mess stops now" :) |
|
Working on testing but if all goes well this PR should be deprecated by: |
|
Testing of #136 worked out. Closing this one. |
Tumor VAF cutoff for FP filter applied to mutect and strelka calls can now be tuned at
immuno.wdllevel usingimmuno.min_var_freqin yaml.immuno.varscan_min_var_freqis used specifically for varscan now.immuno.filter_somatic_llr_thresholdcan be set at the level ofimmuno.wdlin yaml