-
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
Integrate CountingVariantFilter. #4954
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4954 +/- ##
===============================================
- Coverage 86.753% 85.058% -1.695%
+ Complexity 29767 29142 -625
===============================================
Files 1825 1813 -12
Lines 137744 137448 -296
Branches 15181 15579 +398
===============================================
- Hits 119497 116910 -2587
- Misses 12729 15090 +2361
+ Partials 5518 5448 -70
|
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 to me.
9d62bfb
to
167ccf7
Compare
So, the problem with this is that the counting filters use the filter class name to report summaries, and most of the variant filters that we have are implemented using lambdas, so they have no recognizable name. Before this is merged I'll want to reimplement the handful of existing filters used by CNNScoreVariants and Funcotator to use static, named classes in VariantFilterLibrary. |
92490cc
to
8c88874
Compare
8c88874
to
2a34d4b
Compare
@lbergelson I had to add another commit to this after you reviewed it (see my note above), so back to you for another look. |
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.
@cmnbroad This looks good to me 👍
Looks like this needs a rebase, feel free to merge after unless there are crazy changes needed. |
2a34d4b
to
934de50
Compare
934de50
to
ed21e64
Compare
Fixes #4747.