Refactor two VariantEval methods to allow subclasses to override #5998

bbimber · 2019-06-12T17:39:33Z

We have a tool, VariantQC, that extends VariantEval. This PR is a minor refactor to expose the code that creates the list of VariantStratifier and VariantEvaluator objects as protected methods, so subclasses could modify them. This should have no functional difference on VariantEval itself. We're hoping to use these changes in order to adapt our tool in response to reviewers, so if there is any way to push these changes we would appreciate it.

bbimber · 2019-06-12T17:41:49Z

FWIW, @cmnbroad is who I worked with on the earlier VariantEval changes.

codecov · 2019-06-12T18:26:18Z

Codecov Report

Merging #5998 into master will decrease coverage by 6.984%.
The diff coverage is 50%.

@@               Coverage Diff               @@
##              master     #5998       +/-   ##
===============================================
- Coverage     86.929%   79.945%   -6.984%     
+ Complexity     32765     30984     -1781     
===============================================
  Files           2016      2016               
  Lines         151460    151466        +6     
  Branches       16628     16628               
===============================================
- Hits          131663    121090    -10573     
- Misses         13732     24549    +10817     
+ Partials        6065      5827      -238

Impacted Files	Coverage Δ	Complexity Δ
...lkers/varianteval/evaluators/VariantEvaluator.java	`70% <0%> (-12.353%)`	`12 <0> (ø)`
...lbender/tools/walkers/varianteval/VariantEval.java	`88.079% <100%> (+0.04%)`	`109 <1> (+1)`	⬆️
...ls/walkers/varianteval/util/EvaluationContext.java	`76.316% <50%> (-4.24%)`	`12 <1> (ø)`
...dorientation/CollectF1R2CountsIntegrationTest.java	`0.714% <0%> (-99.286%)`	`1% <0%> (-14%)`
...kers/filters/VariantFiltrationIntegrationTest.java	`0.826% <0%> (-99.174%)`	`1% <0%> (-25%)`
.../walkers/bqsr/BaseRecalibratorIntegrationTest.java	`1.031% <0%> (-98.969%)`	`1% <0%> (-7%)`
...s/variantutils/VariantsToTableIntegrationTest.java	`1.042% <0%> (-98.958%)`	`1% <0%> (-21%)`
...ers/vqsr/FilterVariantTranchesIntegrationTest.java	`1.053% <0%> (-98.947%)`	`1% <0%> (-5%)`
...on/FindBreakpointEvidenceSparkIntegrationTest.java	`1.754% <0%> (-98.246%)`	`1% <0%> (-6%)`
...bender/tools/spark/PileupSparkIntegrationTest.java	`2.041% <0%> (-97.959%)`	`2% <0%> (-13%)`
... and 173 more

cmnbroad · 2019-06-14T19:39:38Z

I restarted the travis build since the one failure seems to be an unrelated transient issue.

bbimber · 2019-06-14T20:51:52Z

OK, thanks. I tried to keep edits here limited and protected. I'm happy to describe more about what I'm trying to do in VariantQC if that's helpful.

Also - i have not forgotten about trying to refactor VariantQC to better handle arguments (i.e. dont pass the walker to the VariantEvaluator, and to separate a VariantEvalEngine class, somewhat like VariantAnnotationEngine.

cmnbroad · 2019-06-17T21:13:29Z

@bbimber Yes, it might help if could elaborate a bit on what you're trying to do. Specifically I'd like to find an alternative to getStratifierClasses in VariantEvalUtils that doesn't require handing out a map, which should be fairly easy, but it would help if I knew how you were going to use it. Thanks.

bbimber · 2019-06-18T04:56:45Z

@cmnbroad : if getStratifierClasses() is the only sticking point, we can drop that.

Stepping back: as you probably know we have a tool, VariantQC, which basically sets up a number of instances of VariantEval and uses them to aggregate data as it iterates a VCF. this allows the tool to capture data aggregated/stratified at multiple levels with one pass through the VCF.

There are two related aims:

my tool needs to know the allowable stratification classes. Instead of copy/paste the reflection code to find classes, this PR was exposing that getter as a public method. I'm not sure I understand why this is a sticking point, but we can remove this without that much problem for me. Like we discussed earlier, VariantEval should get refactored into a walker class and some kind of VariantEvalEngine class, and this refactor might be a time to address my concern here. This is not actively blocking us. I could also make this a protected getter on VariantEval if the public aspect is what you dont like.
The remaining changes serve a different purpose. Most VariantEvaluator classes are 'dumb' in that they are instantiated with no configuration and always aggregate the same fields. In our tool, we wanted to let the user specify a list of INFO fields to aggregate (i.e. we dont know the target until runtime). We wrote an InfoFieldAggregator class, which is instantiated with the name of an INFO field. This lets our code create multiple instances of that VariantEvaluator, potentially summarizing different fields. The remaining changes are designed to enable this.

cmnbroad

A few simple requests.

cmnbroad · 2019-06-19T12:54:49Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/varianteval/VariantEval.java

            stratManager.set(i, ec);
        }
    }

+    protected EvaluationContext getEvaluationContext(final Set<Class<? extends VariantEvaluator>> evaluationObjects) {


Maybe this would be better named createEvaluationContext, and add javadoc describing what implementers should do, i.e. that it should always create a new instance (unless there is some compelling reason to be more permissive?).

cmnbroad · 2019-06-19T12:55:36Z

...ava/org/broadinstitute/hellbender/tools/walkers/varianteval/evaluators/VariantEvaluator.java

@@ -11,6 +11,10 @@
    private VariantEval walker;
    private final String simpleName;

+    protected VariantEvaluator(String simpleName) {
+        this.simpleName = simpleName;
+    }


The other constructor (below) should delegate to this one now: this(getClass().getSimpleName()), and this should have javadoc.

good point, but getClass() cant be called before the supertype constructor is called? is there a way around that?

I think it should be fine.

cmnbroad · 2019-06-19T13:53:12Z

...ain/java/org/broadinstitute/hellbender/tools/walkers/varianteval/util/EvaluationContext.java

-    private final ArrayList<VariantEvaluator> evaluationInstances;
-    private final Set<Class<? extends VariantEvaluator>> evaluationClasses;
+    protected final List<VariantEvaluator> evaluationInstances;
+    protected final Set<Class<? extends VariantEvaluator>> evaluationClasses;


Lets keep these private and add protected getEvaluationClasses and getEvaluationInstances methods to return them, with javadoc that includes stating that the return values can be null.

ok. is there a reason GATK doesnt use @nullable more?

ok, i got it now, disregard the question about @nullable here. nonetheless, that annotation does seem under-used across GATK

cmnbroad · 2019-06-19T13:53:35Z

...ain/java/org/broadinstitute/hellbender/tools/walkers/varianteval/util/EvaluationContext.java

@@ -26,7 +27,7 @@ public EvaluationContext(final VariantEval walker, final Set<Class<? extends Var
    private EvaluationContext(final VariantEval walker, final Set<Class<? extends VariantEvaluator>> evaluationClasses, final boolean doInitialize) {
        this.walker = walker;
        this.evaluationClasses = evaluationClasses;
-        this.evaluationInstances = new ArrayList<VariantEvaluator>(evaluationClasses.size());
+        this.evaluationInstances = new ArrayList<>(evaluationClasses.size());


cmnbroad · 2019-06-19T15:11:07Z

...main/java/org/broadinstitute/hellbender/tools/walkers/varianteval/util/VariantEvalUtils.java

+     *
+     * @return An unmodifiable map of all VariantStratifier classes
+     */
+    public static Map<String, Class<? extends VariantStratifier>> getStratifierClasses() {


Since this isn't required, lets remove it. To answer your question, I would generally avoid handing out internal structures like this, especially Maps, since its not clear from the types what it contains. There are several alternative ways to do this, and I know this code already does similar things all over the place (extreme example being bindVariantContexts), but lets not expose any more if we can avoid it. As you mentioned, the refactoring is the right way to address it - I think that would be a fair amount of work, but if we want to continue to make changes to this code it will probably be necessary.

bbimber · 2019-06-19T15:57:32Z

@cmnbroad thanks for the quick review - i just pushed those changes.

cmnbroad · 2019-06-20T11:36:16Z

Restarting tests...

bbimber · 2019-06-20T17:16:52Z

@cmnbroad are these codecov results an actual problem or something incorrect in how it's running?

cmnbroad

I wouldn't worry about the code cov diffs.

cmnbroad · 2019-06-20T11:43:39Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/varianteval/VariantEval.java

+     * @param evaluationObjects
+     * @return
+     */
+    protected EvaluationContext createEvaluationContext(final Set<Class<? extends VariantEvaluator>> evaluationObjects) {


Its pro forma, but please add descriptions of the param and return.

cmnbroad · 2019-06-20T19:53:46Z

...ava/org/broadinstitute/hellbender/tools/walkers/varianteval/evaluators/VariantEvaluator.java

@@ -11,6 +11,10 @@
    private VariantEval walker;
    private final String simpleName;

+    protected VariantEvaluator(String simpleName) {
+        this.simpleName = simpleName;
+    }


I think it should be fine.

cmnbroad · 2019-06-20T19:54:25Z

...ain/java/org/broadinstitute/hellbender/tools/walkers/varianteval/util/EvaluationContext.java

+    public Set<Class<? extends VariantEvaluator>> getEvaluationClasses() {
+        return evaluationClasses;
+    }
+


Fill out @return.

bbimber · 2019-06-20T20:02:06Z

@cmnbroad OK, that's added. commits also squashed

cmnbroad · 2019-06-21T13:07:02Z

Thanks @bbimber.

cmnbroad self-assigned this Jun 12, 2019

bbimber force-pushed the VariantEvalMethods branch from c382256 to 8bc60eb Compare June 13, 2019 18:23

bbimber force-pushed the VariantEvalMethods branch from c9936e1 to a4c2910 Compare June 14, 2019 20:49

cmnbroad requested changes Jun 19, 2019

View reviewed changes

cmnbroad requested changes Jun 20, 2019

View reviewed changes

Refactor VariantEval methods to allow subclass to override

fb3421a

bbimber force-pushed the VariantEvalMethods branch from f5ccb87 to fb3421a Compare June 20, 2019 20:01

cmnbroad approved these changes Jun 21, 2019

View reviewed changes

cmnbroad merged commit 68715da into broadinstitute:master Jun 21, 2019

bbimber deleted the VariantEvalMethods branch June 21, 2019 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor two VariantEval methods to allow subclasses to override #5998

Refactor two VariantEval methods to allow subclasses to override #5998

bbimber commented Jun 12, 2019

bbimber commented Jun 12, 2019

codecov bot commented Jun 12, 2019 •

edited

Loading

cmnbroad commented Jun 14, 2019

bbimber commented Jun 14, 2019

cmnbroad commented Jun 17, 2019 •

edited

Loading

bbimber commented Jun 18, 2019

cmnbroad left a comment

cmnbroad Jun 19, 2019

bbimber Jun 19, 2019

cmnbroad Jun 19, 2019

bbimber Jun 19, 2019

cmnbroad Jun 20, 2019

cmnbroad Jun 19, 2019

bbimber Jun 19, 2019

bbimber Jun 19, 2019

cmnbroad Jun 19, 2019

cmnbroad Jun 19, 2019

bbimber Jun 19, 2019

bbimber commented Jun 19, 2019

cmnbroad commented Jun 20, 2019

bbimber commented Jun 20, 2019

cmnbroad left a comment

cmnbroad Jun 20, 2019

bbimber Jun 20, 2019

cmnbroad Jun 20, 2019

cmnbroad Jun 20, 2019

bbimber commented Jun 20, 2019

cmnbroad commented Jun 21, 2019

Refactor two VariantEval methods to allow subclasses to override #5998

Refactor two VariantEval methods to allow subclasses to override #5998

Conversation

bbimber commented Jun 12, 2019

bbimber commented Jun 12, 2019

codecov bot commented Jun 12, 2019 • edited Loading

Codecov Report

cmnbroad commented Jun 14, 2019

bbimber commented Jun 14, 2019

cmnbroad commented Jun 17, 2019 • edited Loading

bbimber commented Jun 18, 2019

cmnbroad 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

bbimber commented Jun 19, 2019

cmnbroad commented Jun 20, 2019

bbimber commented Jun 20, 2019

cmnbroad 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

bbimber commented Jun 20, 2019

cmnbroad commented Jun 21, 2019

codecov bot commented Jun 12, 2019 •

edited

Loading

cmnbroad commented Jun 17, 2019 •

edited

Loading