VQSR serialized model sets annotation order #3655

lucidtronix · 2017-10-03T15:10:46Z

This addresses the problem where serialized GMMs for VQSR assumed the annotation order would be the same between the commands that generated them and the commands that used them. VQSR no longer depends on the commandline order of the annotations.

codecov-io · 2017-10-03T16:48:19Z

Codecov Report

Merging #3655 into master will decrease coverage by 6.756%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##              master   #3655       +/-   ##
=============================================
- Coverage     86.657%   79.9%   -6.756%     
+ Complexity     29049   17191    -11858     
=============================================
  Files           1808    1067      -741     
  Lines         134688   62513    -72175     
  Branches       14938   10166     -4772     
=============================================
- Hits          116716   49948    -66768     
+ Misses         12559    8609     -3950     
+ Partials        5413    3956     -1457

Impacted Files	Coverage Δ	Complexity Δ
...lbender/tools/walkers/vqsr/VariantDataManager.java	`69.068% <100%> (+0.983%)`	`89 <0> (+6)`	⬆️
...bender/tools/walkers/vqsr/VariantRecalibrator.java	`60.57% <100%> (+1.347%)`	`65 <6> (+7)`	⬆️
...nder/engine/datasources/ReferenceHadoopSource.java	`0% <0%> (-100%)`	`0% <0%> (-3%)`
.../tools/spark/sv/discovery/BreakEndVariantType.java	`0% <0%> (-92.381%)`	`0% <0%> (-14%)`
...pleNovelAdjacencyAndChimericAlignmentEvidence.java	`24.324% <0%> (-63.176%)`	`5% <0%> (-5%)`
...walkers/genotyper/GenotypingGivenAllelesUtils.java	`28.571% <0%> (-32.967%)`	`2% <0%> (ø)`
...hellbender/tools/spark/pipelines/SortSamSpark.java	`70.588% <0%> (-29.412%)`	`4% <0%> (-2%)`
...ignment/AssemblyContigWithFineTunedAlignments.java	`70% <0%> (-22.248%)`	`33% <0%> (-26%)`
...itute/hellbender/tools/funcotator/Funcotation.java	`33.333% <0%> (-22.222%)`	`3% <0%> (-2%)`
...decs/xsvLocatableTable/XsvLocatableTableCodec.java	`63.492% <0%> (-19.224%)`	`14% <0%> (-46%)`
... and 1025 more

lbergelson · 2017-10-16T19:41:21Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/vqsr/VariantDataManager.java

@@ -56,7 +56,7 @@ public void setNormalization(final Map<String, Double> anMeans, final Map<String
        return data;
    }

-    public void normalizeData(final boolean calculateMeans) {
+    public void normalizeData(final boolean calculateMeans, List<Integer> theOrder) {


this needs javadoc

droazen · 2017-10-16T19:45:01Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibrator.java

@@ -487,6 +486,25 @@ else if (null != sequenceDictionary) {
        }
    }

+
+    private void orderAndValidateAnnotations(final GATKReportTable annotationTable){


Can you please add an integration test that fails before this fix, and passes after it? Also should add a basic unit test for the new orderAndValidateAnnotations() method.

droazen · 2017-10-16T19:46:00Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibrator.java

+        for (int i = 0; i < annotationTable.getNumRows(); i++){
+            String serialAnno = (String)annotationTable.get(i, "Annotation");
+            for (int j = 0; j < dataManager.annotationKeys.size(); j++) {
+                if (serialAnno.equals( dataManager.annotationKeys.get(j))){


Can you do a hash lookup against the dataManager to avoid an n^2 algorithm? (might not matter if these are small, in which case never mind)

Yeah I was being lazy, but I've never seen us use more than 7 annotations.

droazen

Requested some tests

lucidtronix · 2017-10-18T19:40:53Z

Added unit and integration tests. Had to make a few things protected for testing and added a vcf with a single SNP (where @ldgauthier found this bug) for the integration test. Back to you @droazen.

magicDGS

After #3475, conflicts are expected in the test classes, because they are now extending GATKBaseTest - a rebase should solve most of the problems without need of change of the code (except comments below)

magicDGS · 2017-11-02T09:58:37Z

...org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibratorModelOutputUnitTest.java

@@ -1,7 +1,9 @@
 package org.broadinstitute.hellbender.tools.walkers.vqsr;


After #3475, this should extend GATKBaseTest

ldgauthier · 2018-04-02T16:46:54Z

@lucidtronix here's a blast from the past. You're going to have to rebase and I hope you don't have too many merge conflicts.

@droazen while you're doing your Monday issue review, can you refresh your memory on this one?

lucidtronix · 2018-04-03T22:01:36Z

@droazen back to you

lucidtronix · 2018-08-23T20:53:12Z

@droazen bump

droazen

Assuming that the integration test you added fails before this fix, I am happy (modulo a trivial comment below on a stray import statement).

If @ldgauthier is also happy with this PR you can go ahead and merge when ready.

droazen · 2018-08-24T17:51:28Z

...org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibratorModelOutputUnitTest.java

@@ -1,7 +1,9 @@
 package org.broadinstitute.hellbender.tools.walkers.vqsr;

+import avro.shaded.com.google.common.collect.Lists;


I think you want com.google.common.collect.Lists here.

ldgauthier

I remember being happy with this back in October. I gave Sam the data for the integration test because it was being handled incorrectly. There's also the GATKBaseTest comment to address, but that should be easy peasy.

droazen self-assigned this Oct 16, 2017

lbergelson reviewed Oct 16, 2017

View reviewed changes

droazen reviewed Oct 16, 2017

View reviewed changes

droazen assigned lucidtronix and unassigned droazen Oct 16, 2017

droazen suggested changes Oct 16, 2017

View reviewed changes

magicDGS reviewed Nov 2, 2017

View reviewed changes

lucidtronix force-pushed the sf_vqsr_annotation_order branch from 6a21375 to a15ec43 Compare April 2, 2018 23:36

vqsr serialized model sets annotation order

9944059

lucidtronix force-pushed the sf_vqsr_annotation_order branch from e22d867 to 9944059 Compare August 23, 2018 20:17

droazen approved these changes Aug 24, 2018

View reviewed changes

droazen requested a review from ldgauthier August 24, 2018 17:54

ldgauthier approved these changes Aug 24, 2018

View reviewed changes

lucidtronix added 2 commits August 27, 2018 12:28

respond to review

e159766

semi colon

132e458

lucidtronix merged commit d2e2580 into master Aug 27, 2018

lucidtronix deleted the sf_vqsr_annotation_order branch August 27, 2018 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VQSR serialized model sets annotation order #3655

VQSR serialized model sets annotation order #3655

lucidtronix commented Oct 3, 2017

codecov-io commented Oct 3, 2017 •

edited

Loading

lbergelson Oct 16, 2017

lucidtronix Oct 18, 2017

droazen Oct 16, 2017

droazen Oct 16, 2017 •

edited

Loading

lucidtronix Oct 18, 2017

droazen left a comment

lucidtronix commented Oct 18, 2017

magicDGS left a comment

magicDGS Nov 2, 2017

ldgauthier commented Apr 2, 2018

lucidtronix commented Apr 3, 2018

lucidtronix commented Aug 23, 2018

droazen left a comment

droazen Aug 24, 2018

ldgauthier left a comment

		@@ -1,7 +1,9 @@
		package org.broadinstitute.hellbender.tools.walkers.vqsr;

		@@ -1,7 +1,9 @@
		package org.broadinstitute.hellbender.tools.walkers.vqsr;

		import avro.shaded.com.google.common.collect.Lists;

VQSR serialized model sets annotation order #3655

VQSR serialized model sets annotation order #3655

Conversation

lucidtronix commented Oct 3, 2017

codecov-io commented Oct 3, 2017 • edited Loading

Codecov Report

lbergelson Oct 16, 2017

Choose a reason for hiding this comment

lucidtronix Oct 18, 2017

Choose a reason for hiding this comment

droazen Oct 16, 2017

Choose a reason for hiding this comment

droazen Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

lucidtronix Oct 18, 2017

Choose a reason for hiding this comment

droazen left a comment

Choose a reason for hiding this comment

lucidtronix commented Oct 18, 2017

magicDGS left a comment

Choose a reason for hiding this comment

magicDGS Nov 2, 2017

Choose a reason for hiding this comment

ldgauthier commented Apr 2, 2018

lucidtronix commented Apr 3, 2018

lucidtronix commented Aug 23, 2018

droazen left a comment

Choose a reason for hiding this comment

droazen Aug 24, 2018

Choose a reason for hiding this comment

ldgauthier left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 3, 2017 •

edited

Loading

droazen Oct 16, 2017 •

edited

Loading