-
Notifications
You must be signed in to change notification settings - Fork 594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GL-18 Funcotation filter for homvar & compound hets on two autosomal recessive genes #5843
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5843 +/- ##
===============================================
- Coverage 87.064% 87.058% -0.005%
- Complexity 32123 32206 +83
===============================================
Files 1977 1984 +7
Lines 147336 147583 +247
Branches 16199 16227 +28
===============================================
+ Hits 128276 128483 +207
- Misses 13154 13177 +23
- Partials 5906 5923 +17
|
@als364 Can I request that you title your pull requests more descriptively? Ie., include the tool involved in the PR title, and a concise one-line description of the improvements/fixes. This would help when writing GATK release notes, as we use the PR titles as the basis for initial release notes. Thanks! |
Yes, can do. |
@@ -213,10 +231,56 @@ private VariantContext applyFilters(final VariantContext variant, final Set<Stri | |||
return variantContextBuilder.make(); | |||
} | |||
|
|||
private void buildArHetByGene(VariantContext variant) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in its own class?. Having AR specific code in the FilterFuncotations
class seems like it breaks encapsulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it could be. There's not currently a 'good' place for it as this can't be a FuncotationFilter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - it may be worth designing another class hierarchy for filters that require two passes. That way the next time someone has to make a filter that requires work on the first pass it'll be easier.
Maybe as a subclass of FuncotationFilter
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good.
I had a couple of comments - one about a refactor (similar to what @tlangs suggested), and a few others about some coding conventions we typically use for GATK tools.
src/main/java/org/broadinstitute/hellbender/tools/funcotator/FilterFuncotations.java
Outdated
Show resolved
Hide resolved
@@ -213,10 +231,56 @@ private VariantContext applyFilters(final VariantContext variant, final Set<Stri | |||
return variantContextBuilder.make(); | |||
} | |||
|
|||
private void buildArHetByGene(VariantContext variant) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - it may be worth designing another class hierarchy for filters that require two passes. That way the next time someone has to make a filter that requires work on the first pass it'll be easier.
Maybe as a subclass of FuncotationFilter
?
src/main/java/org/broadinstitute/hellbender/tools/funcotator/FilterFuncotations.java
Outdated
Show resolved
Hide resolved
...main/java/org/broadinstitute/hellbender/tools/funcotator/filtrationRules/ArHomvarFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/funcotator/FilterFuncotations.java
Outdated
Show resolved
Hide resolved
...n/java/org/broadinstitute/hellbender/tools/funcotator/filtrationRules/FuncotationFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Just a couple of comments. Once they're addressed and tests pass, feel free to merge.
src/main/java/org/broadinstitute/hellbender/tools/funcotator/FilterFuncotations.java
Show resolved
Hide resolved
import java.util.stream.Stream; | ||
|
||
public class FilterFuncotationsUtils { | ||
public static Stream<Set<Map.Entry<String, String>>> getTranscriptFuncotations(final FuncotationMap funcotationMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add Javadoc for this method?
} | ||
|
||
private boolean arHetvarRule(Set<Map.Entry<String, String>> funcotations, VariantContext variant) { | ||
return arCompoundHetVariants.stream().anyMatch(hetVariant -> variantContextsMatch(hetVariant, variant)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add in a warning/error here (or in the parent class) if the firstPassApply
and/or afterFirstPass
was never called.
&& v1.getStart() == v2.getStart() | ||
&& v1.getEnd() == v2.getEnd() | ||
&& v1.getReference() == v2.getReference() | ||
&& v1.getReference() == v2.getReference() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this line is a copy/paste error - was there something else you wanted to test between the variant contexts?
This PR adds a new filter for
FilterFuncotations
. For two autosomal recessive genes,MUTYH
andATP7B
, homozygous variants and compound heterozygous variants will be tagged and added to the output vcf.