added --inverted-read-filter argument to allow for selecting reads failing read filters from the command line easily #8724
Conversation
…s from the command line easily
jamesemery
changed the title
jamesemery
changed the title
| @@ -23,6 +24,11 @@ public class DefaultGATKReadFilterArgumentCollection extends GATKReadFilterArgum | |||
| doc="Read filters to be applied before analysis", optional=true, common = true) | |||
| public final List<String> userEnabledReadFilterNames = new ArrayList<>(); // preserve order | |||
|
|
|||
| @Argument(fullName = ReadFilterArgumentDefinitions.INVERTED_READ_FILTER_LONG_NAME, | |||
There was a problem hiding this comment.
Ah, I thought you were going to implement special handling of not in the read filter name which sounded complicated. This is much clearer.
There was a problem hiding this comment.
I considered it but when we add a read filter named Not___ReadFilter i really didn't want to deal with NotNot___ReadFilters cropping up...
There was a problem hiding this comment.
It does mean that you can't interleave the inverted filters with regular filters in any arbitrary order however...
There was a problem hiding this comment.
We could potentially allow !ReadFilter because ! shouldn't be allowed in read filter names, but yeah, it would be more complicated I think.
There was a problem hiding this comment.
@jamesemery This is much simpler than what I was imaginging, good thinking. Could you add tests cases for disabled/inverted interactions? I don't trust that there isn't some weird issue there that we don't notice. Other than that, just some documentation issues.
.../java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterArgumentCollection.java
Show resolved
Hide resolved
...rg/broadinstitute/hellbender/cmdline/GATKPlugin/DefaultGATKReadFilterArgumentCollection.java
Show resolved
Hide resolved
...in/java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterPluginDescriptor.java
Outdated
Show resolved
Hide resolved
...in/java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterPluginDescriptor.java
Outdated
Show resolved
Hide resolved
...in/java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterPluginDescriptor.java
Outdated
Show resolved
Hide resolved
| @@ -538,6 +538,27 @@ public void testEnableDisableConflict() { | |||
| "--" + ReadFilterArgumentDefinitions.DISABLE_READ_FILTER_LONG_NAME, "GoodCigarReadFilter"}); | |||
| } | |||
|
|
|||
| @Test(expectedExceptions = CommandLineException.class) | |||
There was a problem hiding this comment.
I might either add a new more specific exception type or scrape the message. It's really easy to tests like this failing for other reasons like typos and not the one you expect.
There was a problem hiding this comment.
idk... commandline is a very specfic exception and i'm not super inclined to split the exception up into dozens of hyper specific commandline exceptions. Furthermore.... if you look up in the class this is already all over the place in these tests. No action
There was a problem hiding this comment.
I'm in favor of hyper specific exceptions all over the place but that's fine. Did you check that it's actually erroring for the reasons you expect? I says this from experience constructing commandline tests that turned out to be typos and not valid tests.
| @@ -538,6 +538,27 @@ public void testEnableDisableConflict() { | |||
| "--" + ReadFilterArgumentDefinitions.DISABLE_READ_FILTER_LONG_NAME, "GoodCigarReadFilter"}); | |||
| } | |||
|
|
|||
| @Test(expectedExceptions = CommandLineException.class) | |||
There was a problem hiding this comment.
I'm not 100% sure, but I think there are issues with inverted and disabled read filters. You should be allowed to
a) disable the tool defaults and add back one of them but inverted.
b) disable a specific read filter and add it back inverted
Could you add tests for those cases?
There was a problem hiding this comment.
I will add a test for A, B is very thorny... i'm going to say thats an edge case and not handle it as it is a whole lot more complicated for a very niche case (if you want to invert a default filter you must construct your own filter list)
There was a problem hiding this comment.
Ok, that seems fine. I doubt we'll ever have someone complain.
|
Additional comment from @jamesemery
|
…for output display purposes better
…nction as intended now
|
@lbergelson back to you. Responded to everything with one pushback... The --disable-tool-default + inverte mode was broken and i've fixed it. And a fix to make the not filters display correctly on the commandline. |
|
I thank there is a very low chance that our users would be able to find or even notice if we add |
A quick and dirty implementation. The on thing that needs extra scrutiny is going to be the various conditions/failure states in the validate method. I think i caught most of the cases but there might be a gap somewhere.