Add splitting for allele-specific annotations and ADs to VariantsToTable #5697

ldgauthier · 2019-02-20T21:13:27Z

With allele-specific annotations and filtering in the new exome pipeline, this should be helpful. (I was using it for debugging merging Mutect2 perAlleleAnnotations for MT data.)

codecov-io · 2019-02-21T16:44:45Z

Codecov Report

Merging #5697 into master will increase coverage by 0.078%.
The diff coverage is 92.537%.

@@               Coverage Diff               @@
##              master     #5697       +/-   ##
===============================================
+ Coverage     86.999%   87.077%   +0.078%     
+ Complexity     32110     31929      -181     
===============================================
  Files           1974      1942       -32     
  Lines         147249    146962      -287     
  Branches       16218     16249       +31     
===============================================
- Hits          128105    127970      -135     
+ Misses         13236     13070      -166     
- Partials        5908      5922       +14

Impacted Files	Coverage Δ	Complexity Δ
...s/variantutils/VariantsToTableIntegrationTest.java	`100% <100%> (ø)`	`22 <1> (+1)`	⬆️
...er/tools/walkers/variantutils/VariantsToTable.java	`93.519% <90.741%> (+0.337%)`	`88 <11> (+13)`	⬆️
...ute/hellbender/utils/reference/ReferenceUtils.java	`64.706% <0%> (-11.765%)`	`4% <0%> (ø)`
...tute/hellbender/utils/fragments/FragmentUtils.java	`83.784% <0%> (-10.094%)`	`6% <0%> (-6%)`
...plotypecaller/HaplotypeCallerGenotypingEngine.java	`79.747% <0%> (-10.084%)`	`38% <0%> (-16%)`
...kers/haplotypecaller/AssemblyBasedCallerUtils.java	`80.303% <0%> (-8.658%)`	`41% <0%> (-74%)`
.../basicshortmutpileup/BetaBinomialDistribution.java	`65.385% <0%> (-7.343%)`	`5% <0%> (ø)`
...lkers/contamination/MinorAlleleFractionRecord.java	`84.783% <0%> (-6.522%)`	`5% <0%> (-3%)`
...park/pipelines/PrintReadsSparkIntegrationTest.java	`88.532% <0%> (-4.325%)`	`31% <0%> (+29%)`
.../tools/walkers/mutect/SomaticGenotypingEngine.java	`90.05% <0%> (-3.499%)`	`76% <0%> (+6%)`
... and 168 more

takutosato

Back to you @ldgauthier

takutosato · 2019-02-25T19:24:43Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

@@ -102,6 +116,7 @@
 )
 @DocumentedFeature
 public final class VariantsToTable extends VariantWalker {
+    private static final VariantsToTable instance = new VariantsToTable();


Come back to this

Since I killed the noop unit tests I could get rid of this.

takutosato · 2019-02-25T19:26:13Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

+    @Argument(shortName="ASF", doc="The name of an allele-specific INFO field to be split if present", optional=true)
+    private List<String> asFieldsToTake = new ArrayList<>();
+
+    @Argument(shortName="ASGF", doc="The name of an allele-specific INFO field to be split if present", optional=true)


INFO -> FORMAT

Good catch!

takutosato · 2019-02-25T19:33:23Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

                    throw new UserException("There are no samples and no fields - no output will be produced");
                }
            }
        }

+        if (asGenotypeFieldsToTake.isEmpty() && asFieldsToTake.isEmpty() && (!splitMultiAllelic)) {
+            logger.warn("Allele-specific fields will only be split if splitting multi-allelic variants is specified (`--split-multi-allelics` or `-SMA`");


The actual argument is "--split-multi-allelic" singular. Better to extract a variable to avoid something like this.

takutosato · 2019-02-25T19:34:28Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

                    throw new UserException("There are no samples and no fields - no output will be produced");
                }
            }
        }

+        if (asGenotypeFieldsToTake.isEmpty() && asFieldsToTake.isEmpty() && (!splitMultiAllelic)) {


no need for parentheses around (!splitMultiAllelic)

takutosato · 2019-02-25T19:47:38Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

            final String genotypeHeader = createGenotypeHeader();
-            final String separator = (!baseHeader.isEmpty() && !genotypeHeader.isEmpty()) ? "\t" : "";
-            outputStream.println(baseHeader + separator + genotypeHeader);
+            String line = "";


I think this series of if blocks can be shortened (and made clearer) by combining the lists first, as follows:

final List<String> fields = new ArrayList<>(); fields.addAll(fieldsToTake); fields.addAll(asFieldsToTake); final String header = new StringBuilder(Utils.join("\t", fields)) .append("\t") .append(createGenotypeHeader()) .toString(); outputStream.println(header);

Done -- thanks for the snippet

takutosato · 2019-02-25T20:00:49Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java


        final int numRecordsToProduce = splitMultiAllelic ? vc.getAlternateAlleles().size() : 1;
        final List<List<String>> records = new ArrayList<>(numRecordsToProduce);

        final int numFields;
-        final boolean addGenotypeFields = genotypeFieldsToTake != null && !genotypeFieldsToTake.isEmpty();
+        final boolean hasGFields = genotypeFieldsToTake != null && !genotypeFieldsToTake.isEmpty();


genotypeFiledsToTake defaults to an empty array, so you don't need to check that it's not null. I would do away with hasGFields and hasASGFields and just say final boolean addGenotypeFields = !genotypeFieldsToTake.isEmpty() || !asGenotypeFieldsToTake.isEmpty();

takutosato · 2019-02-25T20:07:53Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

-        final boolean addGenotypeFields = genotypeFieldsToTake != null && !genotypeFieldsToTake.isEmpty();
+        final boolean hasGFields = genotypeFieldsToTake != null && !genotypeFieldsToTake.isEmpty();
+        final boolean hasASGFields = asGenotypeFieldsToTake != null && !asGenotypeFieldsToTake.isEmpty();
+        final boolean addGenotypeFields = hasGFields || hasASGFields;
        if ( addGenotypeFields ) {
            numFields = fieldsToTake.size() + genotypeFieldsToTake.size() * samples.size();


Technically speaking you want to add the number of allele specific fields to numFields, but it's not an error because numFields is just an initial list size. In my opinion getting the right initial list size here isn't worth four lines of code.

Excellent point.

takutosato · 2019-02-25T20:28:22Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

+     * @param result the cummulative output
+     * @param alleleCount scalar, R-type or A-type values
+     */
+    private static void addAlleleSpecificFieldValue(final Object val, final List<List<String>> result, final VCFHeaderLineCount alleleCount) {


If you have bandwidth/will power, it would be great if you could revamp this legacy code that uses instanceof and write separate methods that take parameters of different types.

I do not. (You'll note that the date on the unit test file was June of last year.) Ordinarily I loathe instanceof, but after vacillating for a while, I decided to let it ride because performance isn't really an issue here.

Haha sounds good

takutosato · 2019-02-25T20:32:24Z

...t/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTableUnitTest.java

+
+    @Test
+    public void testExtractFields() throws Exception {
+        final VariantsToTable VTTtest = VariantsToTable.getInstance();


I want to say you can replace this with new VariantsToTable().

But really the root of the problem is that extractFields is an instance method when it should be a static method that takes as parameters lists that are currently instance variables such as genotypeFieldsToTake

Given that I never actually tested anything, I just took out the whole class.

takutosato · 2019-02-25T20:38:47Z

...t/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTableUnitTest.java

+                .chr("1").start(15L).stop(15L)
+                .attribute(GATKVCFConstants.AS_RAW_RMS_MAPPING_QUALITY_KEY, "400.00|285.00|385.00")
+                .genotypes(genotype)
+                .make();


Neither test checks for correctness (i.e. uses e.g. Assert.assertTrue) but I think they should.

I had big plans for more functionality, but decided I don't want users mucking around in the GVCFs. I removed these tests.

ldgauthier

Comments addressed. Back to you @takutosato

ldgauthier · 2019-03-01T21:28:13Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

+     * @param result the cummulative output
+     * @param alleleCount scalar, R-type or A-type values
+     */
+    private static void addAlleleSpecificFieldValue(final Object val, final List<List<String>> result, final VCFHeaderLineCount alleleCount) {


I do not. (You'll note that the date on the unit test file was June of last year.) Ordinarily I loathe instanceof, but after vacillating for a while, I decided to let it ride because performance isn't really an issue here.

ldgauthier · 2019-03-01T21:29:05Z

...t/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTableUnitTest.java

+
+    @Test
+    public void testExtractFields() throws Exception {
+        final VariantsToTable VTTtest = VariantsToTable.getInstance();


Given that I never actually tested anything, I just took out the whole class.

ldgauthier · 2019-03-01T21:29:26Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

-        final boolean addGenotypeFields = genotypeFieldsToTake != null && !genotypeFieldsToTake.isEmpty();
+        final boolean hasGFields = genotypeFieldsToTake != null && !genotypeFieldsToTake.isEmpty();
+        final boolean hasASGFields = asGenotypeFieldsToTake != null && !asGenotypeFieldsToTake.isEmpty();
+        final boolean addGenotypeFields = hasGFields || hasASGFields;
        if ( addGenotypeFields ) {
            numFields = fieldsToTake.size() + genotypeFieldsToTake.size() * samples.size();


Excellent point.

ldgauthier · 2019-03-01T21:30:00Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

                    throw new UserException("There are no samples and no fields - no output will be produced");
                }
            }
        }

+        if (asGenotypeFieldsToTake.isEmpty() && asFieldsToTake.isEmpty() && (!splitMultiAllelic)) {
+            logger.warn("Allele-specific fields will only be split if splitting multi-allelic variants is specified (`--split-multi-allelics` or `-SMA`");


ldgauthier · 2019-03-01T21:30:02Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

            final String genotypeHeader = createGenotypeHeader();
-            final String separator = (!baseHeader.isEmpty() && !genotypeHeader.isEmpty()) ? "\t" : "";
-            outputStream.println(baseHeader + separator + genotypeHeader);
+            String line = "";


Done -- thanks for the snippet

ldgauthier · 2019-03-01T21:30:07Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java


        final int numRecordsToProduce = splitMultiAllelic ? vc.getAlternateAlleles().size() : 1;
        final List<List<String>> records = new ArrayList<>(numRecordsToProduce);

        final int numFields;
-        final boolean addGenotypeFields = genotypeFieldsToTake != null && !genotypeFieldsToTake.isEmpty();
+        final boolean hasGFields = genotypeFieldsToTake != null && !genotypeFieldsToTake.isEmpty();


ldgauthier · 2019-03-01T21:30:14Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

                    throw new UserException("There are no samples and no fields - no output will be produced");
                }
            }
        }

+        if (asGenotypeFieldsToTake.isEmpty() && asFieldsToTake.isEmpty() && (!splitMultiAllelic)) {


ldgauthier · 2019-03-01T21:30:22Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

+    @Argument(shortName="ASF", doc="The name of an allele-specific INFO field to be split if present", optional=true)
+    private List<String> asFieldsToTake = new ArrayList<>();
+
+    @Argument(shortName="ASGF", doc="The name of an allele-specific INFO field to be split if present", optional=true)


Good catch!

ldgauthier · 2019-03-01T21:30:42Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTable.java

@@ -102,6 +116,7 @@
 )
 @DocumentedFeature
 public final class VariantsToTable extends VariantWalker {
+    private static final VariantsToTable instance = new VariantsToTable();


Since I killed the noop unit tests I could get rid of this.

ldgauthier · 2019-03-01T21:31:50Z

...t/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VariantsToTableUnitTest.java

+                .chr("1").start(15L).stop(15L)
+                .attribute(GATKVCFConstants.AS_RAW_RMS_MAPPING_QUALITY_KEY, "400.00|285.00|385.00")
+                .genotypes(genotype)
+                .make();


I had big plans for more functionality, but decided I don't want users mucking around in the GVCFs. I removed these tests.

takutosato

Looks good to me!

ldgauthier · 2019-03-25T14:52:29Z

@nh13 in light of your per-allele list subsetting bug report, this may be of interest

ldgauthier requested a review from takutosato February 22, 2019 14:26

takutosato self-assigned this Feb 22, 2019

takutosato requested changes Feb 25, 2019

View reviewed changes

ldgauthier commented Mar 4, 2019

View reviewed changes

takutosato approved these changes Mar 12, 2019

View reviewed changes

Add splitting for allele-specific annotations and ADs to VariantsToTable

ea0cbe9

ldgauthier force-pushed the ldg_allelesForVariantsToTable branch from b1ad96b to ea0cbe9 Compare March 15, 2019 18:18

ldgauthier merged commit 0e36825 into master Mar 25, 2019

ldgauthier deleted the ldg_allelesForVariantsToTable branch March 25, 2019 14:53

ldgauthier mentioned this pull request Sep 5, 2019

Seems VariantsToTable not properly handle AD greater than 100 #6115

Closed

Add splitting for allele-specific annotations and ADs to VariantsToTable #5697

Add splitting for allele-specific annotations and ADs to VariantsToTable #5697

Conversation

ldgauthier commented Feb 20, 2019

codecov-io commented Feb 21, 2019 • edited Loading

Codecov Report

takutosato 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldgauthier 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

takutosato left a comment

Choose a reason for hiding this comment

ldgauthier commented Mar 25, 2019 • edited Loading

codecov-io commented Feb 21, 2019 •

edited

Loading

ldgauthier commented Mar 25, 2019 •

edited

Loading