Fix NPE in VariantEval when run with no intervals/reference #6283
Conversation
lbergelson
commented
Nov 25, 2019
lbergelson
commented
- Fixes an NPE that happened when using certain evaluators with VariantEval and not specifying a reference or intervals
- Fixes NullPointerException error while running VariantEval #6212
| @@ -488,6 +492,7 @@ public void onComplete() { | |||
|
|
|||
| @Override | |||
| public void apply(VariantContext variant, ReadsContext readsContext, ReferenceContext referenceContext, FeatureContext featureContext) { | |||
| referencePositionsCoveredByVariants += variant.getLengthOnReference(); | |||
There was a problem hiding this comment.
Won't you double-count territory for overlapping variants? Does this matter?
There was a problem hiding this comment.
As discussed in person, it might be preferable, instead of returning a nonsense value here, to add a requiresReference() (or similar) method to the VariantEvaluator class, interrogate the selected evaluations in onTraversalStart() in VariantEval, and throw an error if any of the selected evaluations require a reference.
* Fixes an NPE that happened when using certain evaluators with VariantEval and not specifying a reference or intervals * Fixes #6212
lbergelson
force-pushed
the
lb_variant_eval_npe
branch
from
9fddf2f to
5bcfdca
Compare
|
@droazen I responded to your comments. This is way to much code to deal with this trivial edge case, but now it's Done Right™ |
There was a problem hiding this comment.
Two minor comments to address @lbergelson , otherwise looks good. Feel free to merge after addressing them.
| } | ||
|
|
||
| private void assertThatTerritoryIsSpecifiedIfNecessary() { | ||
| final Set<String> evaluatorsWichRequireTerritory = stratManager.values() |
| @@ -0,0 +1,18 @@ | |||
| package org.broadinstitute.hellbender.tools.walkers.varianteval.evaluators; | |||
There was a problem hiding this comment.
I'm concerned that this test class will be picked up by the reflection code in the main tool and exposed to the user, since it's in the standard evaluator package.
There was a problem hiding this comment.
That's a reasonable concern, but this is in the test code, so it should not be available to the main tool at runtime.
