Added feature to check if input has been annotated #7349

haileypfox · 2021-07-14T19:26:37Z

Added ability for user to override to annotate again

Wrote unit and integration tests for new feature and override ability

resolves #5679

gatk-bot · 2021-07-14T20:18:29Z

Travis reported job failures from build 34953
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	openjdk11	34953.12	logs
integration	openjdk8	34953.2	logs

resolves #7349

gatk-bot · 2021-07-16T16:01:48Z

Travis reported job failures from build 34989
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	openjdk11	34989.12	logs
integration	openjdk8	34989.2	logs

droazen

@haileypfox Back to you with my review comments -- looks good overall, but some minor changes are needed

droazen · 2021-07-16T16:31:25Z

src/main/java/org/broadinstitute/hellbender/tools/funcotator/Funcotator.java

+     *
+     *  No need to annotate again.
+     */
+    static void checkIfAlreadyAnnotated(final VCFHeader vcfHeader) {


If this method has package level access so that test code can access it, it should be annotated with the @VisibleForTesting annotation.

droazen · 2021-07-16T16:32:30Z

src/main/java/org/broadinstitute/hellbender/tools/funcotator/Funcotator.java

+     */
+    static void checkIfAlreadyAnnotated(final VCFHeader vcfHeader) {
+        if (vcfHeader.getOtherHeaderLine("Funcotator") != null) {
+            throw new UserException.BadInput("Given VCF has already been annotated!");


Include the file name / URI in the exception messages

droazen · 2021-07-16T16:33:56Z

src/main/java/org/broadinstitute/hellbender/tools/funcotator/Funcotator.java

+        if (vcfHeader.getOtherHeaderLine("Funcotator") != null) {
+            throw new UserException.BadInput("Given VCF has already been annotated!");
+        }
+        else if (vcfHeader.getOtherHeaderLine("Funcotator Version") != null) {


These header line keys "Funcotator Version" and "Funcotator" should ideally be declared as static final constants somewhere -- possibly in FuncotatorConstants or GATKVCFConstants

droazen · 2021-07-16T16:35:25Z

src/main/java/org/broadinstitute/hellbender/cmdline/StandardArgumentDefinitions.java

@@ -36,6 +36,7 @@ private StandardArgumentDefinitions(){}
    public static final String DISABLE_SEQUENCE_DICT_VALIDATION_NAME = "disable-sequence-dictionary-validation";
    public static final String ADD_OUTPUT_SAM_PROGRAM_RECORD = "add-output-sam-program-record";
    public static final String ADD_OUTPUT_VCF_COMMANDLINE = "add-output-vcf-command-line";
+    public static final String REANNOTATE_VCF_LONG_NAME = "reannotate-vcf";


I think the declaration of this constant in FuncotatorArgumentDefinitions below is sufficient, and you don't need to repeat it here in StandardArgumentDefinitions

droazen · 2021-07-16T16:35:55Z

...java/org/broadinstitute/hellbender/tools/funcotator/FuncotatorVariantArgumentCollection.java

@@ -40,4 +41,12 @@
    )
    public int threePrimeFlankSize = FuncotatorArgumentDefinitions.THREE_PRIME_FLANK_SIZE_DEFAULT_VALUE;

+    @Argument(
+            fullName = StandardArgumentDefinitions.REANNOTATE_VCF_LONG_NAME,


FuncotatorArgumentDefinitions

droazen · 2021-07-16T16:51:09Z

src/test/java/org/broadinstitute/hellbender/tools/funcotator/FuncotatorIntegrationTest.java

+                    outputFormatType,
+                    true,
+                    Collections.emptyList(),
+                    true);


Add comment noting that the --reannotate-vcf is being turned on here, to make that clearer for the reader of your code

droazen · 2021-07-16T16:52:13Z

src/test/java/org/broadinstitute/hellbender/tools/funcotator/FuncotatorIntegrationTest.java

+    }
+
+    /** In this instance, we are again giving an already annotated file, but
+     * we are using the flag argument "true" to indicate that we want this file


Mention the --reannotate-vcf argument in this comment

droazen · 2021-07-16T16:53:55Z

src/test/java/org/broadinstitute/hellbender/tools/funcotator/FuncotatorIntegrationTest.java

+     * to be reannotated. There should be no errors.
+     */
+    @Test(dataProvider = "provideForCheckIfAlreadyAnnotatedTestWrong")
+    public void testCheckWhenWantReannotate(final String inputVcfName,


The names of your test methods are a bit hard to understand -- I suggest tweaking the names to make it clearer what configuration each method is running with. Eg.,

testAlreadyAnnotatedInputWithOverrideArgument()
testAlreadyAnnotatedInputWithoutOverrideArgument()

droazen · 2021-07-16T16:56:26Z

src/test/java/org/broadinstitute/hellbender/tools/funcotator/FuncotatorUnitTest.java

+        headerLines.add(new VCFHeaderLine("FORMAT", "FORMAT"));
+        headerLines.add(new VCFHeaderLine("FORMAT", "FORMAT"));
+        headerLines.add(new VCFHeaderLine("FORMAT", "FORMAT"));
+        headerLines.add(new VCFHeaderLine("Funcotator", "Funcotator"));


Add a second test case with a header containing a "Funcotator Version" line, but not a "Funcotator" line, to exercise the else if clause in the method you're testing.

droazen · 2021-07-16T16:56:44Z

src/test/java/org/broadinstitute/hellbender/tools/funcotator/FuncotatorUnitTest.java

+        headerLines.add(new VCFHeaderLine("FILTER", "FILTER"));
+        headerLines.add(new VCFHeaderLine("FORMAT", "FORMAT"));
+        headerLines.add(new VCFHeaderLine("FORMAT", "FORMAT"));
+        headerLines.add(new VCFHeaderLine("FORMAT", "FORMAT"));


You probably don't need quite so many non-Funcotator header lines here -- 4-5 would be sufficient :)

gatk-bot · 2021-07-16T17:18:30Z

Travis reported job failures from build 34993
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	openjdk11	34993.12	logs
integration	openjdk8	34993.2	logs

gatk-bot · 2021-07-16T18:58:08Z

Travis reported job failures from build 34999
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	openjdk11	34999.12	logs
integration	openjdk8	34999.2	logs

gatk-bot · 2021-07-16T18:58:26Z

Travis reported job failures from build 34997
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	openjdk11	34997.12	logs
integration	openjdk8	34997.2	logs

droazen · 2021-07-19T19:47:14Z

@haileypfox You have a failing test in FuncotatorIntegrationTest -- see the test logs in the gatk-bot comment above:

https://storage.googleapis.com/hellbender-test-logs/build_reports/master_34997.12/tests/test/index.html
https://storage.googleapis.com/hellbender-test-logs/build_reports/master_34997.2/tests/test/index.html

Can you fix this test failure before I re-review?

Added ability for user to override to annotate again Wrote unit and integration tests for new feature and override ability resolves #5679

droazen

@haileypfox Latest version looks good, and is passing tests now. Merging!

For future reference, note that in FuncotatorUnitTest, the two test cases provideDataForAnnotationCheckWrong and provideDataForAnnotationCheckWrongSecond should ideally have been combined into a single DataProvider with two test cases, so that the same test method could be used for both.

haileypfox requested a review from jonn-smith July 14, 2021 19:26

haileypfox added a commit that referenced this pull request Jul 16, 2021

Fixing errors in the integration test

8212658

resolves #7349

droazen assigned jonn-smith Jul 16, 2021

haileypfox force-pushed the hf_checkingalreadyannotated branch from 3197e99 to 885bb94 Compare July 16, 2021 16:26

droazen self-requested a review July 16, 2021 16:26

droazen assigned droazen and unassigned jonn-smith Jul 16, 2021

droazen suggested changes Jul 16, 2021

View reviewed changes

droazen assigned haileypfox and unassigned droazen Jul 16, 2021

haileypfox force-pushed the hf_checkingalreadyannotated branch from ce30d6f to 70ead30 Compare July 16, 2021 18:08

Added feature to check if input has been annotated

562e2f3

Added ability for user to override to annotate again Wrote unit and integration tests for new feature and override ability resolves #5679

haileypfox force-pushed the hf_checkingalreadyannotated branch from ee7d126 to 562e2f3 Compare July 19, 2021 20:25

droazen approved these changes Jul 20, 2021

View reviewed changes

droazen merged commit 31a92ba into master Jul 20, 2021

droazen deleted the hf_checkingalreadyannotated branch July 20, 2021 17:02

droazen mentioned this pull request Nov 9, 2021

Align the Funcotator checkIfAlreadyAnnotated test with the Funcotator engine code. #7555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added feature to check if input has been annotated #7349

Added feature to check if input has been annotated #7349

haileypfox commented Jul 14, 2021

gatk-bot commented Jul 14, 2021 •

edited

Loading

gatk-bot commented Jul 16, 2021 •

edited

Loading

droazen left a comment

droazen Jul 16, 2021

droazen Jul 16, 2021

droazen Jul 16, 2021

droazen Jul 16, 2021

droazen Jul 16, 2021

droazen Jul 16, 2021

droazen Jul 16, 2021

droazen Jul 16, 2021

droazen Jul 16, 2021

droazen Jul 16, 2021

gatk-bot commented Jul 16, 2021 •

edited

Loading

gatk-bot commented Jul 16, 2021 •

edited

Loading

gatk-bot commented Jul 16, 2021 •

edited

Loading

droazen commented Jul 19, 2021

droazen left a comment

Added feature to check if input has been annotated #7349

Added feature to check if input has been annotated #7349

Conversation

haileypfox commented Jul 14, 2021

gatk-bot commented Jul 14, 2021 • edited Loading

gatk-bot commented Jul 16, 2021 • edited Loading

droazen 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

gatk-bot commented Jul 16, 2021 • edited Loading

gatk-bot commented Jul 16, 2021 • edited Loading

gatk-bot commented Jul 16, 2021 • edited Loading

droazen commented Jul 19, 2021

droazen left a comment

Choose a reason for hiding this comment

gatk-bot commented Jul 14, 2021 •

edited

Loading

gatk-bot commented Jul 16, 2021 •

edited

Loading

gatk-bot commented Jul 16, 2021 •

edited

Loading

gatk-bot commented Jul 16, 2021 •

edited

Loading

gatk-bot commented Jul 16, 2021 •

edited

Loading