FuncotationMap refactoring and VCF/MAF concordance for protein changes #4838

LeeTL1220 · 2018-05-31T19:52:34Z

Closes #3842
Closes #4808
Closes #4811
Closes #4810
Closes #4839

A lot of refactoring to create FuncotationMap and use that to send to OutputRenderers.
VCF and MAF will honor the canonical vs. all vs best effect.
MAF entries will now be rendered as AnnotatedInterval via tribble

LeeTL1220 · 2018-05-31T19:56:00Z

@jonn-smith Please review that I have tested to your satisfaction. Particularly around FuncotationMap. Please note that several issues are closed with this PR.

codecov-io · 2018-05-31T20:52:10Z

Codecov Report

Merging #4838 into master will increase coverage by 0.062%.
The diff coverage is 87.925%.

@@               Coverage Diff               @@
##              master     #4838       +/-   ##
===============================================
+ Coverage     80.353%   80.416%   +0.062%     
- Complexity     17714     17811       +97     
===============================================
  Files           1088      1089        +1     
  Lines          63985     64133      +148     
  Branches       10313     10340       +27     
===============================================
+ Hits           51414     51573      +159     
+ Misses          8558      8502       -56     
- Partials        4013      4058       +45

Impacted Files	Coverage Δ	Complexity Δ
...itute/hellbender/tools/funcotator/Funcotation.java	`33.333% <ø> (ø)`	`3 <0> (ø)`	⬇️
...ools/funcotator/FuncotatorArgumentDefinitions.java	`86.364% <ø> (-0.87%)`	`1 <0> (ø)`
...te/hellbender/tools/funcotator/OutputRenderer.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...tils/annotatedinterval/AnnotatedIntervalCodec.java	`79.31% <0%> (ø)`	`18 <2> (+1)`	⬆️
...org/broadinstitute/hellbender/engine/GATKTool.java	`90.667% <0%> (-0.405%)`	`99 <0> (ø)`
.../tools/funcotator/mafOutput/MafOutputRenderer.java	`95.64% <100%> (+0.349%)`	`51 <12> (+3)`	⬆️
...cotator/dataSources/vcf/VcfFuncotationFactory.java	`87.013% <100%> (-2.32%)`	`25 <0> (+1)`
...nder/tools/funcotator/TranscriptSelectionMode.java	`89.72% <100%> (-0.757%)`	`1 <0> (ø)`
.../tools/funcotator/vcfOutput/VcfOutputRenderer.java	`85.246% <100%> (+3.428%)`	`12 <3> (+3)`	⬆️
.../tools/funcotator/dataSources/DataSourceUtils.java	`63.241% <100%> (ø)`	`40 <0> (ø)`	⬇️
... and 17 more

jonn-smith

A few questions and requests.

jonn-smith · 2018-06-01T14:36:06Z

src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java

@@ -821,6 +821,17 @@ private String createProgramGroupID(final SAMFileHeader header) {
     */
    protected String getToolkitShortName() { return "GATK"; }

+    /**
+     * Call {@link GATKTool#addFeatureInputsAfterInitialization(String, String, Class, int)} with no caching.
+     *


Could you add javadoc for the args here?

jonn-smith · 2018-06-01T14:52:11Z

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

        }
        else {
            return createDefaultFuncotationsOnVariant(variant, referenceContext);
        }
+


This works fine, but why rearrange it?

The old code could have outputFuncotations of length 0 and return it, but we would have wanted a default funcotation.

This happened when all of the featureList (i.e. transcripts) were IGR, since we filter out the IGR transcripts (a modification I made later per our offline discussion)
No action.

jonn-smith · 2018-06-01T15:52:08Z

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

+    public static FuncotationMap createFromGencodeFuncotations(final List<GencodeFuncotation> gencodeFuncotations) {
+
+        Utils.nonNull(gencodeFuncotations);
+        Utils.validateArg(!areDuplicateTranscriptIDsFound(gencodeFuncotations), "Duplicate transcript ID entries were found in input: " +


This is confusing - are you searching for duplicates or that there were no duplicates?

I am ensuring that there were no duplicates.

I updated the javadoc comment.

Done.

jonn-smith · 2018-06-01T15:53:03Z

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

+     * @param txId  Never {@code null}
+     * @param funcotations  Never {@code null}
+     */
+    public void add(final String txId, final List<Funcotation> funcotations) {


As per discussion in person, reminder to throw an exception if you add a GencodeFuncotation here (for now).

Done and issue filed.

jonn-smith · 2018-06-01T15:55:17Z

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

+        Utils.nonNull(txId);
+        Utils.nonNull(funcotations);
+
+        final LinkedHashSet<Funcotation> existingFuncotationsToUpdate = txToFuncotations.getOrDefault(txId, new LinkedHashSet<>());


Would it be faster to add the new funcotations directly to the mapped list ala containsKey and ...get(txId).append(...)?

I believe it is the same.

No action.

jonn-smith · 2018-06-05T12:49:10Z

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

+        final String[] funcotationKeys = extractFuncotatorKeysFromHeaderDescription(funcotationHeaderLine.getDescription());
+
+        for (final String annotationToCheck: annotationsToCheck) {
+            final List<String> mafProteinChanges = maf.getRecords().stream().map(v -> v.getAnnotationValue(MafOutputRendererConstants.FieldName_Protein_Change)).collect(Collectors.toList());


It seems like you can pull this out of the loop...

This is confusing - you create a list of mafProteinChanges but it doesn't depend on the annotation. How does the assert work?

Fixed. I forgot to make a second list for MAF annotation names that corresponds to the VCF.

Done

jonn-smith · 2018-06-05T12:58:53Z

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

+            final FuncotationMap funcotationMap = alleleToFuncotationMap.values().iterator().next();
+            Assert.assertEquals(funcotationMap.keyList().size(), 1);
+            Assert.assertTrue(funcotationMap.keyList().stream().noneMatch(k -> k.equals(FuncotationMap.NO_TRANSCRIPT_AVAILABLE_KEY)));
+            Assert.assertTrue(funcotationMap.keyList().stream().noneMatch(k -> StringUtils.isEmpty(k)));


You can change this to StringUtils::isEmpty

jonn-smith · 2018-06-05T13:02:44Z

...roadinstitute/hellbender/tools/funcotator/dataSources/vcf/VcfFuncotationFactoryUnitTest.java

@@ -165,19 +165,11 @@ private VariantContext createVariantContext(final String contig,
                                        Allele.create("C"), FACTORY_NAME)
                        )
                ),
-                // Three overlapping VCF features:
+                // No overlapping VCF features, since there are no indels in dbSNP (the test datasource), so the ground truth should be a default entry, which was constructed here manually:


Could you add in a test case here that has 3 overlapping features?

I clarified the comment. Three overlap by position... none overlap if you include ref and alt in the "overlap".

Done.

(I'm assuming that you do not want me to create a VCF datasource with three identical records).

jonn-smith · 2018-06-05T13:08:18Z

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

+
+    private FuncotationMap() {}
+
+    private static boolean isGencodeFuncotation(final Funcotation f) {


This seems like it would make more sense in FuncotatorUtils

Done. Added tests, too.

jonn-smith · 2018-06-05T13:10:52Z

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

-        // Create a place to keep our funcotations:
-        final List<Funcotation> funcotations = new ArrayList<>();
+        // Create only the gencode funcotations.
+        if (retriveGencodeFuncotationFactoryStream().count() > 1) {


Strictly speaking, this isn't the case anymore - you can have as many Gencode data sources as you want, so this may need to be changed.

I worry about errors in the future... such as what do we do if two gencode datasources have the same transcript?

I changed the message to something less ominous: "Attempting to annotate with more than one GENCODE datasource. If these have overlapping transcript IDs, errors may occur."

Done

LeeTL1220 · 2018-06-06T14:53:37Z

@jonn-smith Back at you. I asked a few questions related to some of my responses.

jonn-smith

One minor request here and one in response to prior comments. About to check latest commit.

jonn-smith · 2018-06-07T22:38:30Z

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

+    }
+
+
+    //TODO: Duplicate code


@LeeTL1220 is this TODO addressed?

Nevermind - disregard this.

jonn-smith

One minor request in the responses to your initial comments, then it looks good.

jonn-smith · 2018-06-07T22:50:57Z

@LeeTL1220 Back to you for a minor request.

…mparisons. This is useful for dev testing. - Added parsing of funcotation attribute using the VCF Header. Mostly used for dev testing. - VCF Datasources now have to match the alt and ref alleles to be condsidered a hit. - Fixing #4808 - Added FuncotationMap to improve encapsulation and make rendering a bit easier. - Fixed issue with FuncotatorIntegrationTest where default values were references that were changed, thereby causing cross-test errors. - A lot of refactoring to create FuncotationMap and use that to send to OutputRenderers. - VCF and MAF will honor the canonical vs. all vs best effect.

jonn-smith

Looks good (pending test success).

LeeTL1220 requested a review from jonn-smith May 31, 2018 19:52

jonn-smith self-assigned this May 31, 2018

LeeTL1220 force-pushed the ll_refactoring_for_tx_all_mode branch from 5217116 to 43c5098 Compare May 31, 2018 20:00

LeeTL1220 mentioned this pull request May 31, 2018

Funcotator: deal with delimiters in data for output formats #4693

Closed

jonn-smith requested changes Jun 5, 2018

View reviewed changes

jonn-smith assigned LeeTL1220 and unassigned jonn-smith Jun 5, 2018

LeeTL1220 assigned jonn-smith and unassigned LeeTL1220 Jun 6, 2018

jonn-smith reviewed Jun 7, 2018

View reviewed changes

jonn-smith requested changes Jun 7, 2018

View reviewed changes

jonn-smith assigned LeeTL1220 and unassigned jonn-smith Jun 7, 2018

LeeTL1220 added 4 commits June 7, 2018 21:50

Answering first PR requests.

0b9da6b

Addressed forgotten TODO.

41e10b5

Addressed PR comment

48e6443

LeeTL1220 force-pushed the ll_refactoring_for_tx_all_mode branch from 30f7ad3 to 48e6443 Compare June 8, 2018 01:51

jonn-smith approved these changes Jun 8, 2018

View reviewed changes

LeeTL1220 merged commit 7e5d4c7 into master Jun 8, 2018

LeeTL1220 deleted the ll_refactoring_for_tx_all_mode branch June 8, 2018 14:33

jonn-smith mentioned this pull request Jun 11, 2018

Funcotator - VCFOutputRenderer needs to be reviewed with new changes from MAF #4583

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FuncotationMap refactoring and VCF/MAF concordance for protein changes #4838

FuncotationMap refactoring and VCF/MAF concordance for protein changes #4838

LeeTL1220 commented May 31, 2018 •

edited

Loading

LeeTL1220 commented May 31, 2018

codecov-io commented May 31, 2018 •

edited

Loading

jonn-smith left a comment

jonn-smith Jun 1, 2018

LeeTL1220 Jun 5, 2018

jonn-smith Jun 1, 2018

LeeTL1220 Jun 5, 2018

jonn-smith Jun 1, 2018

LeeTL1220 Jun 5, 2018

jonn-smith Jun 1, 2018

LeeTL1220 Jun 5, 2018

jonn-smith Jun 1, 2018

LeeTL1220 Jun 5, 2018

jonn-smith Jun 5, 2018

LeeTL1220 Jun 6, 2018

jonn-smith Jun 5, 2018

LeeTL1220 Jun 6, 2018

jonn-smith Jun 5, 2018

LeeTL1220 Jun 6, 2018

LeeTL1220 Jun 6, 2018

jonn-smith Jun 5, 2018

LeeTL1220 Jun 6, 2018

jonn-smith Jun 5, 2018

LeeTL1220 Jun 6, 2018

LeeTL1220 commented Jun 6, 2018

jonn-smith left a comment

jonn-smith Jun 7, 2018 •

edited

Loading

jonn-smith left a comment

jonn-smith commented Jun 7, 2018

jonn-smith left a comment •

edited

Loading


		private FuncotationMap() {}

		private static boolean isGencodeFuncotation(final Funcotation f) {

FuncotationMap refactoring and VCF/MAF concordance for protein changes #4838

FuncotationMap refactoring and VCF/MAF concordance for protein changes #4838

Conversation

LeeTL1220 commented May 31, 2018 • edited Loading

LeeTL1220 commented May 31, 2018

codecov-io commented May 31, 2018 • edited Loading

Codecov Report

jonn-smith 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

LeeTL1220 commented Jun 6, 2018

jonn-smith left a comment

Choose a reason for hiding this comment

jonn-smith Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

jonn-smith left a comment

Choose a reason for hiding this comment

jonn-smith commented Jun 7, 2018

jonn-smith left a comment • edited Loading

Choose a reason for hiding this comment

LeeTL1220 commented May 31, 2018 •

edited

Loading

codecov-io commented May 31, 2018 •

edited

Loading

jonn-smith Jun 7, 2018 •

edited

Loading

jonn-smith left a comment •

edited

Loading