Add experimental FilterFuncotations tool #4991

danxmoran · 2018-07-09T13:45:44Z

The tool is currently hard-coded to work with funcotations from v3 of the AoU data source, but opening for early feedback / discussion of the best way to generalize things.

LeeTL1220

@danxmoran Nothing huge, but a bunch of little things.

LeeTL1220 · 2018-07-16T13:52:00Z

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

+        if (funcotationHeaderLine != null) {
+            funcotationKeys = FuncotatorUtils.extractFuncotatorKeysFromHeaderDescription(funcotationHeaderLine.getDescription());
+            outputVcfWriter = createVCFWriter(outputFile);
+            vcfHeader.addMetaDataLine(new VCFFilterHeaderLine(NOT_CLINSIG_FILTER, "Filter for clinically insignificant variants."));


Please make a new constants class for the description and other header info.

LeeTL1220 · 2018-07-16T13:53:02Z

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

+    }
+}
+
+abstract class FuncotationFiltrationRule {


Classes should go in a separate file.

(and separate/sub-package)

LeeTL1220 · 2018-07-16T13:55:37Z

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

+
+    static final String ONE_LINE_SUMMARY = "Filter variants based on clinically-significant Funcotations.";
+    static final String SUMMARY = ONE_LINE_SUMMARY +
+            " Proof-of-concept hard-coded to look for specific Funcotations from ClinVar, ExAC, and LMM.";


Should cite Laboratory for Molecular Medicine (LMM) (http://personalizedmedicine.partners.org/laboratory-for-molecular-medicine/)

And cite the others as well.

Add a doc line that this tool is an example of parsing and using the VCF output of Funcotator.

LeeTL1220 · 2018-07-16T14:00:22Z

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

+            });
+        });
+
+        String clinicalSignificance = matchingFilters.isEmpty() ? "NONE" : String.join(",", matchingFilters);


LeeTL1220 · 2018-07-16T14:01:15Z

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

+        AFR, AMR, EAS, FIN, NFE, OTH, SAS
+    }
+
+    private static Logger logger = LogManager.getLogger(ClinVarFilter.class);


logger should be FuncotationFiltrationRule.class, right? Or you should create a new one in each subclass?

Was copy-paste typo, but removed now

LeeTL1220 · 2018-07-17T13:51:06Z

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

+        return result;
+    }
+
+    double getMaxMinorAlleleFreq(final Map<String, String> funcotations) {


public? private?

Moved, now private

LeeTL1220 · 2018-07-17T13:52:09Z

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

+    }
+
+    @Override
+    public void apply(VariantContext variant, ReadsContext readsContext, ReferenceContext referenceContext, FeatureContext featureContext) {


parameters should be final

LeeTL1220 · 2018-07-17T13:52:26Z

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

+        outputVcfWriter.add(applyFilters(variant));
+    }
+
+    private VariantContext applyFilters(VariantContext variant) {


parameter should be final

LeeTL1220 · 2018-07-17T13:52:49Z

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

+        this.ruleName = ruleName;
+    }
+
+    boolean checkRule(final VariantContext variant,final Map<String, String> prunedTranscriptFuncotations) {


Add space after comma

LeeTL1220 · 2018-07-18T13:07:36Z

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

+        final List<FuncotationFiltrationRule> lofFiltrationRules = new ArrayList<>();
+        // 1) 1) Variant classification is FRAME_SHIFT_*, NONSENSE, START_CODON_DEL, and SPLICE_SITE
+        // (within 2 bases on either side of exon or intron) on any transcript.
+        // TODO


What is the TODO? Can this be removed or addressed?

Leftover, removed

danxmoran · 2018-07-18T21:39:55Z

@LeeTL1220 I think I hit all your comments. While moving stuff around I massaged the abstract rule class into a SAM type, which I think cleaned things up pretty well. I added a bunch of javadoc too.

danxmoran · 2018-07-19T13:16:52Z

...rg/broadinstitute/hellbender/tools/funcotator/filtrationRules/FuncotationFiltrationRule.java

+    /**
+     * Check if a set of Funcotations matches this rule.
+     */
+    boolean checkRule(final Map<String, String> funcotations);


This is basically Predicate<Map<String, String>> now... worth refactoring to just use that type, or is the custom naming helpful for understanding?

Yeah - I say leave it in for now.

LeeTL1220

@danxmoran Apologies, I found a few more small items. Generally, we want all public methods to have complete javadocs, including the parameters (and whether null is accepted, for example). And you can use Utils.nonNull(...) to enforce some of the rules.

LeeTL1220 · 2018-07-19T13:12:46Z

...n/java/org/broadinstitute/hellbender/tools/funcotator/filtrationRules/FuncotationFilter.java

+    /**
+     * Check all of this filter's rules against a set of Funcotations.
+     *
+     * @return true if the Funcotations match all of this filter's rules, and false otherwise


Java doc needs to be complete for a public method.

LeeTL1220 · 2018-07-19T13:13:07Z

.../broadinstitute/hellbender/tools/funcotator/filtrationRules/FilterFuncotationsExacUtils.java

+     * Build a {@link FuncotationFiltrationRule} matching Funcotations with a MAF less than
+     * the given value across all sub-populations of ExAC.
+     */
+    public static FuncotationFiltrationRule buildExacMaxMafRule(final double maxMaf) {


Please finish javadocs.

LeeTL1220 · 2018-07-19T15:47:40Z

src/main/java/org/broadinstitute/hellbender/tools/funcotator/filtrationRules/LofFilter.java

+    /**
+     * Variant classifications which should be matched by this filter.
+     */
+    private static final List<String> CONSTANT_LOF_CLASSIFICATIONS = Arrays.asList(


Can you tie these to the constants actually used by Funcotator?

Specifically you can use the serialized versions of the enums in GencodeFuncotation.VariantClassification (except for START_CODON_DEL).

jonn-smith

A few relatively minor things in addition to @LeeTL1220 comments.

jonn-smith · 2018-07-19T16:43:19Z

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

+            "\n  * Exome Aggregation Consortium (ExAC) (http://exac.broadinstitute.org/)" +
+            "\n  * Laboratory for Molecular Medicine (LMM) (http://personalizedmedicine.partners.org/laboratory-for-molecular-medicine/)";
+
+    public enum ReferenceVersion {


Can you add a javadoc for the public enum?

jonn-smith · 2018-07-19T16:44:39Z

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

+    public enum ReferenceVersion {
+        hg19(19), hg38(27);
+
+        private final int gencodeVersion;


Pinning to an internal gencode version may not be the best idea - some reference versions don't have an official gencode release (like b37).

The mapping doesn't have to be one-to-one, so reference versions without an official gencode release would point at whatever gencode version they do use in Funcotator.

Bigger-picture, the only reason this is needed is to derive names for the gencode funcotations on each variant. hg19 vs. hg38 seemed more user-friendly than taking the raw gencode version (which is buried in the data source). Are there any utils for extracting out the gencode stuff from a FuncotationMap which abstract over the version number? I could find code for building/writing, but not reading/parsing.

@danxmoran Ah, I see.

I don't think there are any utilities for that right now.

In the mean time, can you add an entry in here for the b37 reference? It would correspond to Gencode 19 as well.

jonn-smith · 2018-07-19T19:43:50Z

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

+
+    private VariantContextWriter outputVcfWriter;
+    private String[] funcotationKeys;
+    private List<FuncotationFilter> funcotationFilters = new ArrayList<>();


You can make this final.

jonn-smith · 2018-07-19T19:48:47Z

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

+        // Mark the individual filters that make the variant significant.
+        final String clinicalSignificance = matchingFilters.isEmpty() ?
+                FilterFuncotationsConstants.CLINSIG_INFO_NOT_SIGNIFICANT :
+                String.join(",", matchingFilters);


There should be a constant in the VcfOutputRenderer that you can use instead of hard-coding the comma.

There wasn't one that I could find... should I create one?

@danxmoran Oops. My mistake. Yeah - I think adding one in FilterFuncotations itself is sufficient.

jonn-smith · 2018-07-19T19:51:37Z

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

+        final VariantContextBuilder variantContextBuilder = new VariantContextBuilder(variant);
+
+        // Mark the individual filters that make the variant significant.
+        final String clinicalSignificance = matchingFilters.isEmpty() ?


In this method you check matchingFilters.isEmpty twice. It might simplify the code if you had one if statement.

jonn-smith · 2018-07-19T20:09:02Z

...rg/broadinstitute/hellbender/tools/funcotator/filtrationRules/FuncotationFiltrationRule.java

+    /**
+     * Check if a set of Funcotations matches this rule.
+     */
+    boolean checkRule(final Map<String, String> funcotations);


Yeah - I say leave it in for now.

jonn-smith · 2018-07-19T20:24:54Z

src/main/java/org/broadinstitute/hellbender/tools/funcotator/filtrationRules/LofFilter.java

+    /**
+     * Variant classifications which should be matched by this filter.
+     */
+    private static final List<String> CONSTANT_LOF_CLASSIFICATIONS = Arrays.asList(


Specifically you can use the serialized versions of the enums in GencodeFuncotation.VariantClassification (except for START_CODON_DEL).

jonn-smith · 2018-07-19T20:27:06Z

src/main/java/org/broadinstitute/hellbender/tools/funcotator/filtrationRules/LofFilter.java

+                    return classification.startsWith(FRAME_SHIFT_PREFIX) || CONSTANT_LOF_CLASSIFICATIONS.contains(classification);
+                },
+                funcotations -> funcotations.getOrDefault(LOF_GENE_FUNCOTATION, "").equals("YES"),
+                FilterFuncotationsExacUtils.buildExacMaxMafRule(0.01));


May be worth pulling this constant out into a variable in case it changes down the road.

jonn-smith · 2018-07-19T20:32:45Z

...t/java/org/broadinstitute/hellbender/tools/funcotator/FilterFuncotationsIntegrationTest.java

+                                  final Set<String> expectedFilters,
+                                  final Set<String> expectedAnnotations) throws IOException {
+
+        final Path tmpOut = Files.createTempFile(vcfName + ".filtered", ".vcf");


You can use BaseTest::createTempFile for this - it'll automatically delete the file on exit.

danxmoran · 2018-07-23T12:24:01Z

@LeeTL1220 @jonn-smith I think I covered everything, back to you!

codecov-io · 2018-07-23T13:14:26Z

Codecov Report

Merging #4991 into master will increase coverage by 0.012%.
The diff coverage is 93.893%.

@@               Coverage Diff               @@
##              master     #4991       +/-   ##
===============================================
+ Coverage     86.654%   86.667%   +0.012%     
- Complexity     28997     29045       +48     
===============================================
  Files           1800      1808        +8     
  Lines         134531    134662      +131     
  Branches       14930     14935        +5     
===============================================
+ Hits          116577    116707      +130     
+ Misses         12546     12543        -3     
- Partials        5408      5412        +4

Impacted Files	Coverage Δ	Complexity Δ
.../tools/funcotator/FilterFuncotationsConstants.java	`0% <0%> (ø)`	`0 <0> (?)`
.../funcotator/filtrationRules/FuncotationFilter.java	`100% <100%> (ø)`	`4 <4> (?)`
...ools/funcotator/filtrationRules/ClinVarFilter.java	`100% <100%> (ø)`	`5 <5> (?)`
...er/tools/funcotator/filtrationRules/LmmFilter.java	`100% <100%> (ø)`	`2 <2> (?)`
...er/tools/funcotator/filtrationRules/LofFilter.java	`100% <100%> (ø)`	`5 <5> (?)`
.../funcotator/FilterFuncotationsIntegrationTest.java	`100% <100%> (ø)`	`5 <5> (?)`
...r/filtrationRules/FilterFuncotationsExacUtils.java	`82.353% <82.353%> (ø)`	`7 <7> (?)`
...ellbender/tools/funcotator/FilterFuncotations.java	`93.103% <93.103%> (ø)`	`18 <18> (?)`
... and 8 more

LeeTL1220

@danxmoran One more minor comment. Feel free to merge if Jonn is okay and my last comment is addressed.

I have not commented on how we could do less hardcoding/make this tool more versatile, since that is better handled in an issue. We can discuss whenever you like, here or in person.

LeeTL1220 · 2018-07-24T14:17:23Z

.../broadinstitute/hellbender/tools/funcotator/filtrationRules/FilterFuncotationsExacUtils.java

+     * Build a {@link FuncotationFiltrationRule} matching Funcotations from variants with a
+     * maximum MAF less than some threshold.
+     *
+     * @param maxMaf the MAF threshold to check in the rule


Are there any parameter checks that need to happen here? Such as >0 ?

See ParamUtils ...

jonn-smith

Sorry for the delay. I thought this was going to be accepted in my absence.

Three very minor changes. Good to merge after and when tests pass.

jonn-smith · 2018-08-17T15:36:01Z

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

+     * Used to derive names of Gencode Funcotations.
+     */
+    public enum ReferenceVersion {
+        hg19(19), hg38(27);


As mentioned in an above response to a comment - can you add a b37 reference version here as well? It would correspond to Gencode version 19 as well.

jonn-smith · 2018-08-17T15:36:51Z

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

+                    VCFHeaderLineType.String, FilterFuncotationsConstants.CLINSIG_INFO_KEY_DESCRIPTION));
+            outputVcfWriter.writeHeader(vcfHeader);
+        } else {
+            throw new UserException.BadInput("Input VCF does not have Funcotator annotations.");


Can you mention in this exception that it's an error in the header as opposed to the data itself?

jonn-smith · 2018-08-17T15:38:54Z

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

+
+        // Mark the individual filters that make the variant significant, if any.
+        final String clinicalSignificance =
+                isSignificant ? String.join(",", matchingFilters) : FilterFuncotationsConstants.CLINSIG_INFO_NOT_SIGNIFICANT;


As mentioned in a comment above, can you extract the comma here into a constant in this class?

Co-authored-by: Jay Carey <jcarey@broadinstitute.org>

danxmoran requested review from LeeTL1220 and jonn-smith July 9, 2018 13:45

droazen assigned jonn-smith Jul 9, 2018

LeeTL1220 requested changes Jul 18, 2018

View reviewed changes

danxmoran force-pushed the dm-filter-funcotations-merge-back-DSDEGP-2550 branch from cd1dfc0 to bacc647 Compare July 18, 2018 21:35

danxmoran commented Jul 19, 2018

View reviewed changes

LeeTL1220 requested changes Jul 19, 2018

View reviewed changes

jonn-smith requested changes Jul 19, 2018

View reviewed changes

LeeTL1220 reviewed Jul 26, 2018

View reviewed changes

jonn-smith requested changes Aug 17, 2018

View reviewed changes

jonn-smith assigned danxmoran and unassigned jonn-smith Aug 17, 2018

danxmoran and others added 8 commits August 17, 2018 12:20

Add experimental FilterFuncotations tool.

fa9b6c2

Co-authored-by: Jay Carey <jcarey@broadinstitute.org>

Add multi-transcript and multi-allele tests.

8fb4a98

Move code and add comments from review.

7380c79

Even simpler.

91f5f49

Round two of review comments.

e448ef4

Enforce bounds on max MAF.

4c9274d

Final round of review comments.

e805042

Fix import after rebase.

ab78b93

danxmoran force-pushed the dm-filter-funcotations-merge-back-DSDEGP-2550 branch from b967d91 to ab78b93 Compare August 17, 2018 16:24

danxmoran merged commit 12104fc into master Aug 17, 2018

danxmoran deleted the dm-filter-funcotations-merge-back-DSDEGP-2550 branch August 17, 2018 18:03

Add experimental FilterFuncotations tool #4991

Add experimental FilterFuncotations tool #4991

Conversation

danxmoran commented Jul 9, 2018

LeeTL1220 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danxmoran commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeeTL1220 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

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

danxmoran commented Jul 23, 2018

codecov-io commented Jul 23, 2018 • edited

Codecov Report

LeeTL1220 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

codecov-io commented Jul 23, 2018 •

edited