SelectVariants and VariantFiltration not updating AC, AN and AF for --setFilteredGtToNocall #1871

ronlevine · 2016-06-01T14:41:34Z

Fixes #1731

Summary of changes

SelectVariants.java
- createVCFHeaderLineList() - ensure allele count INFO annotations (AC, AN, AF) are in the output VCF header if --setFilteredGtToNocall
- setFilteredGenotypeToNocall() - recompute AC, AN and AF if filtered genotypes are set to no-call, add them to the genotype builder.
VariantFiltration.java
- initializeVcfWriter() - ensure allele count INFO annotations (AC, AN, AF) are in the output VCF header if --setFilteredGtToNocall
- makeGenotypes() - recompute AC, AN and AF if filtered genotypes are set to no-call, add them to the genotype builder.
GATKVariantContextUtils.java
- incrementChromosomeCountsInfo() - count the total and alternate alleles for a genotype
- updateChromosomeCountsInfo() - update AC, AN and AF with the computed count of total and alternate alleles
selectVariantsInfoField.vcf* and variantFiltrationInfoField.vcf*
- User data from https://github.com/broadinstitute/gsa-unstable/issues/1325
VariantFiltrationIntegrationTest.java
- Tests that show the fix for https://github.com/broadinstitute/gsa-unstable/issues/1325
SelectVariantsIntegrationTest.java
- MD5 changed in testSetFilteredGtoNocall() because AC, AN and AF are always in the VCF if --setFilteredGtToNocall
GATKVariantContextUtilsUnitTest.java
- Test new GATKVariantContextUtils methods

ronlevine · 2016-06-01T14:42:24Z

@droazen Please choose a reviewer.

coveralls · 2016-06-01T15:13:40Z

Coverage increased (+0.04%) to 82.044% when pulling 2b0d306 on rhl_sv_vf_update_ac into 911d5fb on master.

droazen · 2016-06-21T18:17:46Z

@akiezun Would you have time to review this one this week, since you've been looking at Ron's other PRs?

akiezun · 2016-07-14T18:17:18Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/filters/VariantFiltration.java

@@ -177,6 +178,9 @@

    private final List<Allele> diploidNoCallAlleles = Arrays.asList(Allele.NO_CALL, Allele.NO_CALL);

+    // Number of called alleles
+    int calledAlleles = 0;


remove this field. It is state that is not carried over apply calls and thus it is useless. Turn it into a temp and pass directly to methods that need it

Moved to makeGenotypes().

akiezun · 2016-07-14T18:34:14Z

looks good. small edits. back to you @ronlevine. Please rebase before next round.

droazen · 2016-08-03T15:48:55Z

@ronlevine Will you have time to address Adam's comments on this PR?

ronlevine · 2016-08-03T18:34:22Z

Yes. It will have to wait until Monday.

ronlevine · 2016-08-08T17:28:04Z

@droazen Please take a look. Note that I removed the functional programming in SelectVariants.setFilteredGenotypeToNocall(). I could not think of a clean way to implement the added logic.

coveralls · 2016-08-08T17:40:05Z

Coverage increased (+0.03%) to 82.233% when pulling 9ebdaf3 on rhl_sv_vf_update_ac into a830c13 on master.

coveralls · 2016-08-08T17:47:55Z

Coverage increased (+0.03%) to 82.236% when pulling 9ebdaf3 on rhl_sv_vf_update_ac into a830c13 on master.

coveralls · 2016-08-08T17:50:31Z

Coverage increased (+0.03%) to 82.236% when pulling 9ebdaf3 on rhl_sv_vf_update_ac into a830c13 on master.

lbergelson · 2016-08-24T15:50:36Z

src/main/java/org/broadinstitute/hellbender/utils/variant/GATKVariantContextUtils.java

+    public static void updateChromosomeCountsInfo(final Map<Allele, Integer> calledAltAlleles, final int calledAlleles,
+                                                  final VariantContextBuilder builder) {
+        Utils.nonNull(calledAltAlleles, "Called alternate alleles can not be null");
+        Utils.nonNull(builder, "Variant context builder can not be null");


you might want a guard against negative calledAlleles here

lbergelson · 2016-08-24T16:11:10Z

@ronlevine It looks to me that there could be some refactoring done here to separate out the AF update logic into a single function call that could be used in multiple places rather than having two sets of fairly complicated and highly similar logic. Could you see if that's doable?

ronlevine · 2016-09-01T00:28:31Z

@lbergelson I refactored the common logic into a utility class. Please take a look.

ronlevine · 2016-09-02T13:12:36Z

@lbergelson Per your suggestion, I made setFilteredGenotypeToNocall() a void function. This is all set for review.

lbergelson · 2016-09-09T19:09:35Z

src/main/java/org/broadinstitute/hellbender/utils/variant/GATKVariantContextUtils.java

@@ -1349,4 +1351,119 @@ private static int indexOfSameAllele(final VariantContext vc, final Allele allel

        return -1;
    }
+
+    /**
+     * Add chromosome Counds (AC, AN and AF) to the VCF header lines


typo: counds

Is chromosome counts established nomenclature that I'm not aware of? It seems like allele counts is more accurate to me since this is all per allele.

Kind of. There are the classes ChromosomeCounts and ChromosomeCountConstants.

So there are. Wait on doing those renames until I talk with a few people here about what the right name is. I don't want it to be inconsistent, but I think it's confusing how it is at the moment.

Also, I noticed that VariantContextUtils has calculateChromosomeCounts which seems to be very similar to the new methods in this pr. Did we know about those going in to these changes? Are we duplicating code that already exists?

I also noticed that ChromosomeCountConstants seems to be totally redundant with ChromosomeCounts. Could you remove ChromoSomeCountConstants while you're in this code? It looks like add ChromomeCounts should probably just addAll ChromosomeCounts.getDescriptions as well..

Removed getCalledChromosomeCounts and updateChromosomeCountsInfo and replaced the VariantContextUtils .calculateChromosomeCounts.

Removed ChromoSomeCountConstants.

lbergelson · 2016-09-09T21:32:58Z

@ronlevine Sorry for the slow response. I like it much more with the refactoring you did. I had a few minor comments. I want to ask @vdauwera about the names on monday, I find them confusing, but if it's standard nomenclature then we should keep them. So don't do any renaming until I get back to you about that I guess.

lbergelson · 2016-09-12T21:54:30Z

@ronlevine I consulted with Geraldine. Keep calling things chromosome counts. That seems to be the accepted name.

ronlevine · 2016-09-13T18:15:13Z

@lbergelson Incorporated your comments. Back to you.

lbergelson · 2016-09-15T16:10:28Z

@ronlevine Looks good to me. Feel free to merge after squashing/rebasing if there aren't any issues with the rebase. Ignore the codacy failure, it's something I was testing but I don't think it's that useful.

…-setFilteredGtToNocall

coveralls · 2016-09-15T18:49:09Z

Coverage increased (+0.01%) to 79.985% when pulling 88acbda on rhl_sv_vf_update_ac into 4ec1b85 on master.

ronlevine assigned droazen Jun 1, 2016

droazen assigned akiezun and unassigned droazen Jun 21, 2016

akiezun reviewed Jul 14, 2016
View reviewed changes

akiezun assigned ronlevine and unassigned akiezun Jul 14, 2016

ronlevine force-pushed the rhl_sv_vf_update_ac branch 2 times, most recently from 9104222 to 9ebdaf3 Compare August 8, 2016 17:20

ronlevine assigned droazen and unassigned ronlevine Aug 8, 2016

ronlevine added the secon label Aug 8, 2016

droazen assigned lbergelson and unassigned droazen Aug 22, 2016

lbergelson reviewed Aug 24, 2016
View reviewed changes

lbergelson assigned ronlevine and unassigned lbergelson Aug 24, 2016

ronlevine assigned lbergelson and unassigned ronlevine Sep 1, 2016

lbergelson reviewed Sep 9, 2016
View reviewed changes

lbergelson removed their assignment Sep 15, 2016

SelectVariants and VariantFiltration not updating AC, AN and AF for -…

88acbda

…-setFilteredGtToNocall

ronlevine force-pushed the rhl_sv_vf_update_ac branch from b230d3c to 88acbda Compare September 15, 2016 18:19

ronlevine merged commit 7882c82 into master Sep 15, 2016

ronlevine deleted the rhl_sv_vf_update_ac branch September 15, 2016 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SelectVariants and VariantFiltration not updating AC, AN and AF for --setFilteredGtToNocall #1871

SelectVariants and VariantFiltration not updating AC, AN and AF for --setFilteredGtToNocall #1871

ronlevine commented Jun 1, 2016

ronlevine commented Jun 1, 2016

coveralls commented Jun 1, 2016 •

edited

Loading

droazen commented Jun 21, 2016

akiezun Jul 14, 2016 •

edited

Loading

ronlevine Aug 8, 2016 •

edited

Loading

akiezun commented Jul 14, 2016

droazen commented Aug 3, 2016

ronlevine commented Aug 3, 2016

ronlevine commented Aug 8, 2016

coveralls commented Aug 8, 2016

coveralls commented Aug 8, 2016

coveralls commented Aug 8, 2016

lbergelson Aug 24, 2016

ronlevine Aug 31, 2016

lbergelson commented Aug 24, 2016

ronlevine commented Sep 1, 2016

ronlevine commented Sep 2, 2016 •

edited

Loading

lbergelson Sep 9, 2016

lbergelson Sep 9, 2016

ronlevine Sep 9, 2016

lbergelson Sep 9, 2016

ronlevine Sep 13, 2016

lbergelson commented Sep 9, 2016

lbergelson commented Sep 12, 2016

ronlevine commented Sep 13, 2016

lbergelson commented Sep 15, 2016

coveralls commented Sep 15, 2016

SelectVariants and VariantFiltration not updating AC, AN and AF for --setFilteredGtToNocall #1871

SelectVariants and VariantFiltration not updating AC, AN and AF for --setFilteredGtToNocall #1871

Conversation

ronlevine commented Jun 1, 2016

ronlevine commented Jun 1, 2016

coveralls commented Jun 1, 2016 • edited Loading

droazen commented Jun 21, 2016

akiezun Jul 14, 2016 • edited Loading

Choose a reason for hiding this comment

ronlevine Aug 8, 2016 • edited Loading

Choose a reason for hiding this comment

akiezun commented Jul 14, 2016

droazen commented Aug 3, 2016

ronlevine commented Aug 3, 2016

ronlevine commented Aug 8, 2016

coveralls commented Aug 8, 2016

coveralls commented Aug 8, 2016

coveralls commented Aug 8, 2016

lbergelson Aug 24, 2016

Choose a reason for hiding this comment

ronlevine Aug 31, 2016

Choose a reason for hiding this comment

lbergelson commented Aug 24, 2016

ronlevine commented Sep 1, 2016

ronlevine commented Sep 2, 2016 • edited Loading

lbergelson Sep 9, 2016

Choose a reason for hiding this comment

lbergelson Sep 9, 2016

Choose a reason for hiding this comment

ronlevine Sep 9, 2016

Choose a reason for hiding this comment

lbergelson Sep 9, 2016

Choose a reason for hiding this comment

ronlevine Sep 13, 2016

Choose a reason for hiding this comment

lbergelson commented Sep 9, 2016

lbergelson commented Sep 12, 2016

ronlevine commented Sep 13, 2016

lbergelson commented Sep 15, 2016

coveralls commented Sep 15, 2016

coveralls commented Jun 1, 2016 •

edited

Loading

akiezun Jul 14, 2016 •

edited

Loading

ronlevine Aug 8, 2016 •

edited

Loading

ronlevine commented Sep 2, 2016 •

edited

Loading