Add warning message to add allele specific annotation group #3042

ronlevine · 2017-06-06T17:10:52Z

Implements #2983.

codecov-io · 2017-06-06T18:35:16Z

Codecov Report

Merging #3042 into master will increase coverage by 0.41%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##              master     #3042      +/-   ##
==============================================
+ Coverage     86.679%   87.089%   +0.41%     
- Complexity     29668     31524    +1856     
==============================================
  Files           1819      1930     +111     
  Lines         137383    145236    +7853     
  Branches       15166     16096     +930     
==============================================
+ Hits          119082    126485    +7403     
- Misses         12822     12898      +76     
- Partials        5479      5853     +374

Impacted Files	Coverage Δ	Complexity Δ
...ute/hellbender/utils/variant/GATKVCFConstants.java	`75% <ø> (-5%)`	`4 <0> (ø)`
...lkers/ReferenceConfidenceVariantContextMerger.java	`95.313% <100%> (+0.056%)`	`72 <0> (+1)`	⬆️
...itute/hellbender/engine/spark/KnownSitesCache.java	`0% <0%> (-91.667%)`	`0% <0%> (-8%)`
...itute/hellbender/engine/filters/VariantFilter.java	`33.333% <0%> (-33.333%)`	`1% <0%> (-1%)`
...ls/funcotator/metadata/VcfFuncotationMetadata.java	`71.429% <0%> (-28.571%)`	`8% <0%> (+3%)`
...hellbender/tools/walkers/vqsr/CNNVariantTrain.java	`60% <0%> (-20.645%)`	`4% <0%> (ø)`
...ender/engine/spark/datasources/ReadsSparkSink.java	`57.353% <0%> (-19.674%)`	`16% <0%> (-17%)`
...der/engine/spark/datasources/ReadsSparkSource.java	`61.333% <0%> (-18.875%)`	`17% <0%> (-14%)`
...der/tools/HaplotypeCallerSparkIntegrationTest.java	`68.293% <0%> (-18.472%)`	`16% <0%> (ø)`
...pynumber/datacollection/AllelicCountCollector.java	`65.789% <0%> (-14.856%)`	`13% <0%> (ø)`
... and 461 more

ronlevine · 2017-06-06T20:05:33Z

...ava/org/broadinstitute/hellbender/tools/walkers/ReferenceConfidenceVariantContextMerger.java

-                // nothing to do
-                //TODO This comes directly from gatk3, we should determine if we should log this or if it happens so much it is not useful to log
+                final String baseMsg = "Remaining (non-reducible) annotations are assumed to be ints or doubles, but " + value + " doesn't parse and will not be annotated in the final VC.";
+                final String msg = value.toString().contains("|") ? baseMsg + " Add -G Standard -G AS_Standard to the command to annotate in the final VC." : baseMsg;


From @droazen This seems like a pretty brittle way of detecting whether AS annotations are present...

droazen · 2017-06-06T21:16:27Z

@lbergelson please review

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)

jamesemery · 2018-01-12T17:41:43Z

@droazen I updated this branch to the new code. I think its good but just want someone to quickly look at it before t goes in.

jamesemery · 2018-01-18T18:59:56Z

@lbergelson Could you take a quick look at this one? I just updated it to be in line with the changes I've made to our annotaiton framework.

lbergelson · 2018-01-19T21:13:04Z

...ava/org/broadinstitute/hellbender/tools/walkers/ReferenceConfidenceVariantContextMerger.java

@@ -382,6 +385,7 @@ private void addReferenceConfidenceAttributes(final VCWithNewAlleles vcPair, fin
                    values.add(parseNumber(value.toString()));
                } catch (final NumberFormatException e) {
                    warning.warn(String.format("Detected invalid annotations: When trying to merge variant contexts at location %s:%d the annotation %s was not a numerical value and was ignored",vcPair.getVc().getContig(),vcPair.getVc().getStart(),p.toString()));
+                    if (key.startsWith("AS")) AS_warning.warn(String.format("Reducible annotation '%s' detected, add -G Standard -G AS_Standard to the command to annotate in the final VC with this annotation.",key));


I always like {} on if statements even for 1 liners...

lbergelson · 2018-01-19T21:16:29Z

src/test/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFsIntegrationTest.java

@@ -255,4 +255,14 @@ public void testIntervalsAndOnlyOutputCallsStartingInIntervalsAreMutuallyRequire
        args.addArgument("L", "20:69512-69513");
        runCommandLine(args);
    }
+
+    @Test
+    public void testMissingAlleleSpecificAnnotationGroup() throws IOException {


this doesn't really test anything

lbergelson

2 comments and then 👍 to go

droazen · 2019-01-11T16:02:31Z

@jamesemery Can you remove the non-functional test, and merge this once tests pass?

droazen

👍 merge once tests pass

lbergelson

👍

ronlevine requested review from droazen and lbergelson June 6, 2017 17:10

ronlevine self-assigned this Jun 6, 2017

ronlevine commented Jun 6, 2017

View reviewed changes

ronlevine mentioned this pull request Jun 6, 2017

Add warning message to add allele specific annotation group broadinstitute/gatk-protected#1054

Closed

droazen removed their request for review June 6, 2017 21:16

droazen assigned lbergelson and unassigned ronlevine Jun 6, 2017

droazen assigned jamesemery and unassigned lbergelson Sep 18, 2017

droazen requested review from jamesemery and removed request for lbergelson September 18, 2017 19:36

magicDGS reviewed Nov 2, 2017

View reviewed changes

jamesemery force-pushed the rhl_warn_msg_as_group_1037 branch from b9e5e7d to 90cf70a Compare January 11, 2018 16:53

jamesemery requested a review from lbergelson January 18, 2018 18:59

lbergelson reviewed Jan 19, 2018

View reviewed changes

lbergelson requested changes Jan 19, 2018

View reviewed changes

jamesemery force-pushed the rhl_warn_msg_as_group_1037 branch from 90cf70a to 731f80f Compare January 22, 2018 17:24

jamesemery added 4 commits September 14, 2018 16:17

Updating the branch to the current gatk

675aa86

resplonding

2c32e0c

capture stderr

83eb835

updating again

3f7d9b8

jamesemery force-pushed the rhl_warn_msg_as_group_1037 branch from 731f80f to 3f7d9b8 Compare September 14, 2018 20:23

fixing compiler

955eb1f

removed the offending test so the warning message can be added

3d21f49

droazen approved these changes Jan 11, 2019

View reviewed changes

lbergelson approved these changes Jan 11, 2019

View reviewed changes

jamesemery merged commit a74e571 into master Jan 11, 2019

jamesemery deleted the rhl_warn_msg_as_group_1037 branch January 11, 2019 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning message to add allele specific annotation group #3042

Add warning message to add allele specific annotation group #3042

ronlevine commented Jun 6, 2017 •

edited

Loading

codecov-io commented Jun 6, 2017 •

edited

Loading

ronlevine Jun 6, 2017

droazen commented Jun 6, 2017

magicDGS left a comment

jamesemery commented Jan 12, 2018

jamesemery commented Jan 18, 2018

lbergelson Jan 19, 2018

lbergelson Jan 19, 2018

lbergelson left a comment

droazen commented Jan 11, 2019

droazen left a comment

lbergelson left a comment

Add warning message to add allele specific annotation group #3042

Add warning message to add allele specific annotation group #3042

Conversation

ronlevine commented Jun 6, 2017 • edited Loading

codecov-io commented Jun 6, 2017 • edited Loading

Codecov Report

ronlevine Jun 6, 2017

Choose a reason for hiding this comment

droazen commented Jun 6, 2017

magicDGS left a comment

Choose a reason for hiding this comment

jamesemery commented Jan 12, 2018

jamesemery commented Jan 18, 2018

lbergelson Jan 19, 2018

Choose a reason for hiding this comment

lbergelson Jan 19, 2018

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

droazen commented Jan 11, 2019

droazen left a comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

ronlevine commented Jun 6, 2017 •

edited

Loading

codecov-io commented Jun 6, 2017 •

edited

Loading