Adding soft clipped ratio to OverclippedReadFilter #5995

jonn-smith · 2019-06-10T19:53:54Z

Added two new arguments to OverclippedReadFilter that enable filtering based on a ratio of soft clipped reads to total read length.

Necessary for long reads analysis.

Added in tests.

codecov · 2019-06-10T20:34:14Z

Codecov Report

Merging #5995 into master will decrease coverage by 68.497%.
The diff coverage is 7.921%.

@@               Coverage Diff               @@
##              master    #5995        +/-   ##
===============================================
- Coverage     86.927%   18.43%   -68.497%     
+ Complexity     32732     9207     -23525     
===============================================
  Files           2014     2016         +2     
  Lines         151333   151426        +93     
  Branches       16612    16623        +11     
===============================================
- Hits          131549    27908    -103641     
- Misses         13724   120739    +107015     
+ Partials        6060     2779      -3281

Impacted Files	Coverage Δ	Complexity Δ
...lbender/cmdline/ReadFilterArgumentDefinitions.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
.../engine/filters/OverclippedReadFilterUnitTest.java	`5.263% <0%> (-94.737%)`	`1 <0> (-3)`
...llbender/engine/filters/OverclippedReadFilter.java	`13.043% <12.5%> (-86.957%)`	`0 <0> (-9)`
...llbender/engine/filters/SoftClippedReadFilter.java	`14.706% <14.706%> (ø)`	`1 <1> (?)`
.../engine/filters/SoftClippedReadFilterUnitTest.java	`3.571% <3.571%> (ø)`	`1 <1> (?)`
...ls/variant/writers/GVCFBlockCombiningIterator.java	`0% <0%> (-100%)`	`0% <0%> (-1%)`
...nder/tools/copynumber/utils/TagGermlineEvents.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
...r/tools/spark/pathseq/PSBwaArgumentCollection.java	`0% <0%> (-100%)`	`0% <0%> (-1%)`
...titute/hellbender/utils/mcmc/DecileCollection.java	`0% <0%> (-100%)`	`0% <0%> (-5%)`
...ender/tools/readersplitters/ReadGroupSplitter.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
... and 1675 more

jamesemery

A couple of issues, and one or two questions. The bigger question I have however is why continue to use minAlignedBases in the ratio tests at all, especially since it's no longer configurable. You are enforcing that reads below (right now 30) still fail the overclipped filter and then asserting on top of that that their clipping is below your threshold. I would say you have two options:

Make them mutually exclusive, thus if you are in one of the ratio modes the aligned length isn't considered at all. This has the drawback that you can never apply both tests if you want but does away with the hard coded 30 value that should affect the ratio tests.
Make them all apply configurably. I.e. you can run with both a clipping ratio test AND a minimum absolute number of aligned bases test (and perhaps even both clipping ratio and edge clipping ratio). This is doable in this class with relative ease (test just gathers everything at once and decides based on settings which ones to run) but you may also want to pull the ratio tests out into separate filters if you do that.

jamesemery · 2019-06-10T20:05:20Z

src/main/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilter.java

+
+    @Override
+    public boolean test(final GATKRead read) {
+        logger.debug( "About to test read: " + read.toString());


This debug statement is going to be expensive, as it has to call read.toString() for every read which means every read gets parsed completely even if debug is off. Either use the message supplier paradigm or encase this in an if statement.

I'm going to remove all debug logging statements anyway - they were in for... well.. debugging.

jamesemery · 2019-06-10T20:06:31Z

src/main/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilter.java

+    public boolean test(final GATKRead read) {
+        logger.debug( "About to test read: " + read.toString());
+
+        // NOTE: Since we have mutex'd the args for the clipping ratios, we only need to see if they


Add a line about this behavior to the javadoc for this filter as well.

jamesemery · 2019-06-10T20:11:45Z

src/main/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilter.java

+
+        final double softClipRatio = ((double)numSoftClippedBases / (double)totalLength);
+
+        logger.debug( "  Num Soft Clipped Bases: " + numSoftClippedBases );


Again, use suppliers or hide them behind if statements to prevent unnecessary method calls.

Debug statements are getting removed.

jamesemery · 2019-06-10T20:17:04Z

src/main/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilter.java

    static final long serialVersionUID = 1L;
+    private final Logger logger = LogManager.getLogger(this.getClass());
+
+    private static final double DEFAULT_MIN_SOFT_CLIPPED_RATIO = Double.MIN_VALUE;


These default values get filled in on the documentation page here: https://software.broadinstitute.org/gatk/documentation/tooldocs/current/org_broadinstitute_hellbender_engine_filters_OverclippedReadFilter.php meaning that you will get some godawful representation of Double.MIN_VALUE filled in on this page. I would suggest either choosing a more manageable default value like 1.0, or setting this to a Double object and making the default value null so it shows up as such and is not confusing.

Double / null it is!

jamesemery · 2019-06-10T20:17:39Z

src/main/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilter.java

+    private int minAlignedBases = 30;
+
+    @VisibleForTesting
+    @Argument(fullName = "soft-clipped-ratio-threshold",


move these names to constants in ReadFilterArgumentDefinitions

jamesemery · 2019-06-10T20:26:28Z

src/main/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilter.java

+
+    @VisibleForTesting
+    @Argument(fullName = "leading-trailing-soft-clipped-ratio",
+            doc = "Minimum ratio of soft clipped bases (leading / trailing the cigar string) to total bases in read for read to be included.",


Is there any reason to necessarily make these ("leading-trailing-soft-clipped-ratio", "soft-clipped-ratio-threshold") mutually exclusive? It seems like it may be possible that one cares about different values for one or the other.

For my use case there is never a time when you want to do a combined filter on both. I don't think it's useful to have them both on at the same time.

We can fix it if someone ever wants to make this happen together.

jamesemery · 2019-06-10T21:00:45Z

src/main/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilter.java

+        logger.debug( "  Read Filter status: " + (softClipRatio > minimumSoftClippedRatio) );
+        logger.debug( "  Aligned Length: " + alignedLength );
+
+        return (alignedLength >= minAlignedBases) && (softClipRatio > minimumSoftClippedRatio);


This should either be || or you should drop the first clause

jamesemery · 2019-06-10T20:35:45Z

src/test/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilterUnitTest.java

+                                                   final double clipRatio,
+                                                   final boolean expectedResult) {
+
+        final OverclippedReadFilter filter = new OverclippedReadFilter(10);


Good tests.

jamesemery · 2019-06-10T20:45:08Z

src/test/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilterUnitTest.java

+
+        // ---------------------------------------
+        // Min bases test:
+        testData.add(new Object[] { "20S5M", 0.2, false }); // 20/25 = .8


I dont think these tests actually demonstrate the minAlignedBases behavior, as both of them have <30 matching cigar bases.

They were showing that strings below 30 bases will return false even though by the other metric they would pass. The other tests have a smaller minAlignedBases and all pass because the threshold has been hit.

These tests have since been removed anyway because of the new class structure.

jamesemery · 2019-06-10T20:49:27Z

src/main/java/org/broadinstitute/hellbender/engine/filters/OverclippedReadFilter.java

+        logger.debug( "  Read Filter status: " + (softClipRatio > minimumLeadingTrailingSoftClippedRatio) );
+        logger.debug( "  Aligned Length: " + alignedLength );
+
+        return (alignedLength >= minAlignedBases) && (softClipRatio > minimumLeadingTrailingSoftClippedRatio);


If these arguments are really mutually exclusive I wouldn't have both options (minAlignedBases, and minimumLeadingTrailingSoftClippedRatio) apply in every case. Either let them both exist at once (as they are all 3 separate tests).

jamesemery · 2019-06-10T21:08:57Z

Perhaps call the alternative a SoftclipRatioReadFilter or something similar

jonn-smith · 2019-06-13T16:53:14Z

Yeah - I'm gonna pull this out into a new read filter.

Addressed review comments.

jamesemery

One comment about the argument name and then you should feel free to merge.

jamesemery · 2019-06-14T14:53:43Z

src/main/java/org/broadinstitute/hellbender/cmdline/ReadFilterArgumentDefinitions.java

+    public static final String SOFT_CLIPPED_RATIO_THRESHOLD = "soft-clipped-ratio-threshold";
+    public static final String SOFT_CLIPPED_LEADING_TRAILING_RATIO_THRESHOLD = "soft-clipped-leading-trailing-ratio";
+
+    public static final String INVERT_FILTER = "invert-filter";


I would make this "invert-soft-clip-ratio" or something like that to make it a little clearer that its associated with THIS filter not just any filter on the command line

Sure. Really, this should be a feature of every reader. I'll add a ticket for it.

jonn-smith added 2 commits June 7, 2019 16:56

Added in capability to filter based on ratio of soft clipped bases.

09d66d5

Added in filter for leading/trailing soft clips only.

8a5a370

Added in tests.

jonn-smith added the enhancement label Jun 10, 2019

jonn-smith requested a review from jamesemery June 10, 2019 19:53

jamesemery requested changes Jun 10, 2019

View reviewed changes

Refactored filter into new class: SoftClippedReadFilter

9e3f0c2

Addressed review comments.

jamesemery approved these changes Jun 14, 2019

View reviewed changes

jonn-smith added 2 commits June 14, 2019 11:53

Addressing round 2 comments.

a157452

Fixing silly problem.

019f990

jonn-smith merged commit 744987b into master Jun 14, 2019

jonn-smith deleted the jts_overclipped_read_filter_ratio branch June 14, 2019 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding soft clipped ratio to OverclippedReadFilter #5995

Adding soft clipped ratio to OverclippedReadFilter #5995

jonn-smith commented Jun 10, 2019

codecov bot commented Jun 10, 2019 •

edited

Loading

jamesemery left a comment

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery Jun 10, 2019

jonn-smith Jun 13, 2019

jamesemery commented Jun 10, 2019

jonn-smith commented Jun 13, 2019

jamesemery left a comment

jamesemery Jun 14, 2019

jonn-smith Jun 14, 2019


		final double softClipRatio = ((double)numSoftClippedBases / (double)totalLength);

		logger.debug( " Num Soft Clipped Bases: " + numSoftClippedBases );

Adding soft clipped ratio to OverclippedReadFilter #5995

Adding soft clipped ratio to OverclippedReadFilter #5995

Conversation

jonn-smith commented Jun 10, 2019

codecov bot commented Jun 10, 2019 • edited Loading

Codecov Report

jamesemery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesemery commented Jun 10, 2019

jonn-smith commented Jun 13, 2019

jamesemery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2019 •

edited

Loading