exomiser / Exomiser

A Tool to Annotate and Prioritize Exome Variants
https://exomiser.readthedocs.io
GNU Affero General Public License v3.0
202 stars 55 forks source link

remove-off-target-syn option description is confusing #34

Closed buske closed 9 years ago

buske commented 9 years ago

Does specifying this option remove off-target variants, or keep them? And is the default to keep them or remove them? It isn't quite clear.

 -T,--remove-off-target-syn             Keep off-target variants. These
                                        are defined as intergenic,
                                        intronic, upstream, downstream,
                                        synonymous or intronic ncRNA
                                        variants. Default: true
buske commented 9 years ago

A similar issue exists for the pathogenic option:

 -P,--remove-path-filter-cutoff <arg>   Keep all variants, regardless of
                                        predicted pathogenicity or variant
                                        type. Default: false
julesjacobsen commented 9 years ago

Yeah, the -T option is totally confusing, even internally which is likely why it's confusing externally. -T actually removes the target filter - this is usually run by default so it's a negative switch. These things make my head hurt.

Perhaps it would be better to not run this by default and then allow users to specify a list of target types which they want to keep?

julesjacobsen commented 9 years ago

OK we've (Peter, Damian, myself) decided to change the options to:

--keep-off-target          Retains all non-exonic variants.
--keep-non-pathogenic      Retains all non pathogenic variants.

Removing all references to the defaults as I think that further complicated and already confusing situation.

e.g.

To be more conservative and detect possible non-coding or synonymous causative variants the ‘--keep-off-target --keep-non-pathogenic’ options should be added to steps 1-4.
buske commented 9 years ago

I think this is much clearer, thanks! How should this be specified in a batch settings file?

julesjacobsen commented 9 years ago

Glad you like it. That was a **\ to sort out. Seemed so trivial, but I got burned with 'keep' turning into 'remove' in the code and none of it making sense and producing the opposite of what was required in two cases out of three. Why is it that a simple Boolean can be so confusing?!

You can see the new settings files under: /exomiser-cli/src/main/resources/test-*.settings

They replace the old keys with the new ones as used on the command-line. n.b. the values have switched true->false

buske commented 9 years ago

Okay, gotcha! I had looked through the commit and didn't see any edits to the settings file, but for some reason didn't consider that that didn't need to be changed. :)