Preserve VCF fields in MAF output #4872

LeeTL1220 · 2018-06-08T20:51:10Z

Created FuncotationMetadata class. An instance of this will appear as an attribute on all Funcotations. This describes the fields in the Funcotation. This is also useful for when you need to know all possible field names in formats where these may not be specified in every variant (e.g. VCF)
Due to previous bullet point, Funcotator will now throw an exception if an attribute in a variant is not in the header.
For now, FuncotationMetadata only supports INFO fields in a VCF.
tumor_f was being mapped to an incorrect field. This has been removed, but not fixed. See Funcotator - Must implement t_alt_count / t_ref_count, alt_count/ref_count #4634 and Does FuncotationMetadata need to support FORMAT fields in a VCF input/datasource? #4871
Sanitizing spaces in funcotation field values.
Fixes Funcotator - Must preserve other VCF fields / annotations in both MAF and VCF #3895
Fix where other transcript field was being populated incorrectly.
(Dev) Creating a Funcotation for the input VCF INFO attributes.
(Dev) Made the VcfOutputRenderer ignore VCF input funcotations.

codecov-io · 2018-06-11T20:47:32Z

Codecov Report

Merging #4872 into master will increase coverage by 0.106%.
The diff coverage is 58.929%.

@@               Coverage Diff               @@
##              master     #4872       +/-   ##
===============================================
+ Coverage     80.453%   80.559%   +0.106%     
- Complexity     17815     18060      +245     
===============================================
  Files           1089      1092        +3     
  Lines          64168     64761      +593     
  Branches       10338     10491      +153     
===============================================
+ Hits           51625     52171      +546     
- Misses          8486      8513       +27     
- Partials        4057      4077       +20

Impacted Files	Coverage Δ	Complexity Δ
...tils/annotatedinterval/AnnotatedIntervalCodec.java	`79.31% <ø> (ø)`	`18 <0> (ø)`	⬇️
...itute/hellbender/tools/funcotator/Funcotation.java	`33.333% <ø> (ø)`	`3 <0> (ø)`	⬇️
...ataSources/xsv/SimpleKeyXsvFuncotationFactory.java	`87.097% <100%> (ø)`	`27 <0> (ø)`	⬇️
...dataSources/gencode/GencodeFuncotationBuilder.java	`96.774% <100%> (+0.053%)`	`30 <1> (ø)`	⬇️
...dataSources/gencode/GencodeFuncotationFactory.java	`83.634% <100%> (+0.025%)`	`164 <5> (+1)`	⬆️
...ncotator/mafOutput/MafOutputRendererConstants.java	`98.98% <100%> (ø)`	`1 <0> (ø)`	⬇️
...ataSources/xsv/LocatableXsvFuncotationFactory.java	`85.185% <100%> (ø)`	`24 <0> (ø)`	⬇️
...te/hellbender/tools/funcotator/FuncotationMap.java	`84.211% <100%> (ø)`	`28 <0> (ø)`	⬇️
.../tools/funcotator/vcfOutput/VcfOutputRenderer.java	`85.714% <100%> (+0.468%)`	`16 <4> (+4)`	⬆️
...ava/org/broadinstitute/hellbender/utils/Utils.java	`80.476% <100%> (+0.235%)`	`145 <3> (+3)`	⬆️
... and 24 more

jonn-smith

Some relatively minor comments and a few questions.

jonn-smith · 2018-06-11T18:15:50Z

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

@@ -250,57 +252,57 @@
            optional = true,
            doc = "Ignore/drop variants that have been filtered in the input.  These variants will not appear in the output file."
    )
-    protected boolean removeFilteredVariants = false;
+    private boolean removeFilteredVariants = false;

    @Argument(
            fullName  = FuncotatorArgumentDefinitions.TRANSCRIPT_SELECTION_MODE_LONG_NAME,
            optional = true,
            doc = "Method of detailed transcript selection.  This will select the transcript for detailed annotation (CANONICAL or BEST_EFFECT)."


Can you update the doc here to include the new ALL option?

jonn-smith · 2018-06-11T18:17:34Z

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

@@ -228,19 +230,19 @@
            fullName =  FuncotatorArgumentDefinitions.REFERENCE_VERSION_LONG_NAME,
            doc = "The version of the Human Genome reference to use (e.g. hg19, hg38, etc.).  This will correspond to a sub-folder of each data source corresponding to that data source for the given reference."
    )
-    protected String referenceVersion;
+    private String referenceVersion;


Why the change to private for the input args? (I have no objection.)

Got rid of IntelliJ warnings.

No action.

jonn-smith · 2018-06-11T18:22:03Z

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

@@ -2022,7 +2024,7 @@ public TranscriptCodingSequenceException( final String msg, final Throwable thro
     */
    public static String sanitizeFuncotationForVcf(final String individualFuncotation) {
        Utils.nonNull(individualFuncotation);
-        return StringUtils.replaceEach(individualFuncotation, new String[]{",", ";", "=", "\t", "|"}, new String[]{"_%2C_", "_%3B_", "_%3D_", "_%09_", "_%7C_"});
+        return StringUtils.replaceEach(individualFuncotation, new String[]{",", ";", "=", "\t", "|", " "}, new String[]{"_%2C_", "_%3B_", "_%3D_", "_%09_", "_%7C_", "_%20_"});


Since the VCF files are TSVs, do we need to escape a space in the values?

I was getting errors when I left it in...

No action.

jonn-smith · 2018-06-11T18:27:10Z

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

@@ -2075,5 +2077,43 @@ public static boolean isGencodeFuncotation(final Funcotation f) {
    public static boolean areAnyGencodeFuncotation(final List<Funcotation> funcotations) {
        return funcotations.stream().anyMatch(FuncotatorUtils::isGencodeFuncotation);
    }
+
+    /**
+     * Create funcotations (one for each alt allele) corresponding to the given variant context.


Can you make this more descriptive? I understand what this is doing but the documentation here is pretty sparse.

jonn-smith · 2018-06-11T18:37:31Z

...roadinstitute/hellbender/tools/funcotator/dataSources/gencode/GencodeFuncotationBuilder.java

@@ -29,6 +31,8 @@ public GencodeFuncotationBuilder() {
    }

    public GencodeFuncotation build() {
+        // TODO: File an issue to give mechanism for populating the metadata  (https://github.com/broadinstitute/gatk/issues/4857)


Comment can be updated since issue now exists.

jonn-smith · 2018-06-11T18:59:41Z

...va/org/broadinstitute/hellbender/tools/funcotator/dataSources/vcf/VcfFuncotationFactory.java

        }
    }

+    @VisibleForTesting
+    static String determineFinalFieldName(final String funcotationFactoryName, final String fieldName) {


This could even be shorter if named createFinalFieldName.

jonn-smith · 2018-06-11T19:01:24Z

src/main/java/org/broadinstitute/hellbender/tools/funcotator/mafOutput/MafOutputRenderer.java

+    /**
+     * @return Immutable copy of the output field map used by this MafOutputRenderer.
+     */
+    public ImmutableMap<String, List<String>> getOutputFieldNameMap() {


This doesn't seem to be used anywhere.

Deleted.

Done

jonn-smith · 2018-06-11T20:11:33Z

...n/java/org/broadinstitute/hellbender/tools/funcotator/metadata/FuncotationMetadataUtils.java

+    public final static String UNKNOWN_DESCRIPTION = "Unknown";
+
+    /**
+     * @param fieldNames Never {@code null}


Can you complete the Javadoc?

Done and added null check.

jonn-smith · 2018-06-11T20:12:23Z

...ain/java/org/broadinstitute/hellbender/tools/funcotator/metadata/VcfFuncotationMetadata.java

+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+public class VcfFuncotationMetadata implements FuncotationMetadata {


Can you add Javadoc for the class?

jonn-smith · 2018-06-12T15:56:53Z

...itute/hellbender/tools/funcotator/dataSources/gencode/GencodeFuncotationFactoryUnitTest.java

@@ -1367,7 +1369,16 @@ void testCreateFuncotations(final String expectedGeneName,
                    genomeChangeCorrect && strandCorrect && cDnaChangeCorrect && codonChangeCorrect && proteinChangeCorrect,
                    errorMessageStringBuilder.toString() + "\n"
            );
-
+


Can you refactor these test cases to be similar to the test cases above (checks with individual error messages and one assert at the end)? That way if there is any regression it is clear how many and which of the checks fail.

I think I kept up with the spirit of this comment, but I did implement it slightly different.

@jonn-smith

Done, unless I misinterpreted.

jonn-smith · 2018-06-12T16:32:00Z

Back to you, @LeeTL1220

…endering. This is very useful for internal consistency. Made TableFuncotation constructors private, since the logic may get more complex. Putting unknown metadata into TableFuncotations. Populating metadata for Gencode Funcotations (unknown) and adding a test. Added space to the disallowed characters to be sanitized, since it was causing other parsing code in the GATK to fail. Fixed regression where test was looking for a space, which is disallowed. Fixed issue where an extra field was being rendered if afuncotation factory had no annotations. Adding test for FuncotationMetadata generated from VCFs. Addressed testing of other fields in the metadata from a VcfFuncotationFactory Added a reverse map utility and added it to MafOutputRenderer so that we can get aliasing from unit tests.

…MAT.

LeeTL1220 · 2018-06-13T22:13:57Z

@jonn-smith Back to you.... Tell me when/if I can merge.

jonn-smith

@LeeTL1220 looks good. Merge away when tests are good.

LeeTL1220 · 2018-06-14T19:54:16Z

@jonn-smith I'll fix the tests and notify you.

…t had "END" column which was throwing off MAF processing.

LeeTL1220 · 2018-06-15T01:58:40Z

@jonn-smith Assuming the tests pass, please review latest changes. If the tests have failed, I will see it tomorrow and address.

jonn-smith

One minor change then I think it looks good.

jonn-smith · 2018-06-15T14:05:13Z

...roadinstitute/hellbender/tools/funcotator/dataSources/gencode/GencodeFuncotationFactory.java

@@ -1934,7 +1934,7 @@ private GencodeFuncotation createFuncotationForSpanningDeletion(final VariantCon
     * @param refAllele The reference {@link Allele} for this variant.
     * @param altAllele The alternate {@link Allele} for this variant.
     * @return A {@link GencodeFuncotation.VariantType} representing the variation type between the given reference and alternate {@link Allele}.
-     * Spanning deletions and no calls will get a type of {@link org.broadinstitute.hellbender.tools.funcotator.dataSources.gencode.GencodeFuncotation.VariantType.NA}
+     * Spanning deletions and no calls will get a type of NA


I liked having the actual link to the NA VariantType here.

LeeTL1220 · 2018-06-15T15:33:15Z

Offline...Was given permission to merge if tests pass.

LeeTL1220 requested a review from jonn-smith June 8, 2018 20:51

LeeTL1220 mentioned this pull request Jun 8, 2018

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

Closed

jonn-smith self-assigned this Jun 11, 2018

jonn-smith requested changes Jun 12, 2018

View reviewed changes

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

jonn-smith mentioned this pull request Jun 12, 2018

Funcotator - Multialleleic variants have merged funcotations only on the last allele #3896

Closed

LeeTL1220 added 4 commits June 13, 2018 18:10

Minor change to fix some rebase issues.

acd0cfd

Removed AF from tumor_f, since it was coming from INFO instead of FOR…

32be909

…MAT.

Answering PR comments.

ade975d

LeeTL1220 force-pushed the ll_VCF_MAF_field_preservation branch from de2200e to ade975d Compare June 13, 2018 22:11

jonn-smith approved these changes Jun 13, 2018

View reviewed changes

Several fixes discovered post-rebase onto master.

d4611d9

Adding workaround to address a unit test that was using test data tha…

29d2a1d

…t had "END" column which was throwing off MAF processing.

jonn-smith requested changes Jun 15, 2018

View reviewed changes

Fixed javadoc

d2ab1eb

LeeTL1220 merged commit 41788f5 into master Jun 15, 2018

LeeTL1220 deleted the ll_VCF_MAF_field_preservation branch June 15, 2018 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve VCF fields in MAF output #4872

Preserve VCF fields in MAF output #4872

LeeTL1220 commented Jun 8, 2018 •

edited

Loading

codecov-io commented Jun 11, 2018 •

edited

Loading

jonn-smith left a comment

jonn-smith Jun 11, 2018

jonn-smith Jun 11, 2018

LeeTL1220 Jun 13, 2018

jonn-smith Jun 11, 2018

LeeTL1220 Jun 13, 2018

jonn-smith Jun 11, 2018

LeeTL1220 Jun 13, 2018

jonn-smith Jun 11, 2018

LeeTL1220 Jun 13, 2018

jonn-smith Jun 11, 2018

LeeTL1220 Jun 13, 2018

jonn-smith Jun 11, 2018

LeeTL1220 Jun 13, 2018

jonn-smith Jun 11, 2018

LeeTL1220 Jun 13, 2018

jonn-smith Jun 11, 2018

LeeTL1220 Jun 13, 2018

jonn-smith Jun 12, 2018

LeeTL1220 Jun 13, 2018

jonn-smith commented Jun 12, 2018

LeeTL1220 commented Jun 13, 2018 •

edited

Loading

jonn-smith left a comment

LeeTL1220 commented Jun 14, 2018

LeeTL1220 commented Jun 15, 2018

jonn-smith left a comment

jonn-smith Jun 15, 2018

LeeTL1220 commented Jun 15, 2018

Preserve VCF fields in MAF output #4872

Preserve VCF fields in MAF output #4872

Conversation

LeeTL1220 commented Jun 8, 2018 • edited Loading

codecov-io commented Jun 11, 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

jonn-smith commented Jun 12, 2018

LeeTL1220 commented Jun 13, 2018 • edited Loading

jonn-smith left a comment

Choose a reason for hiding this comment

LeeTL1220 commented Jun 14, 2018

LeeTL1220 commented Jun 15, 2018

jonn-smith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeeTL1220 commented Jun 15, 2018

LeeTL1220 commented Jun 8, 2018 •

edited

Loading

codecov-io commented Jun 11, 2018 •

edited

Loading

LeeTL1220 commented Jun 13, 2018 •

edited

Loading