-
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
Refactor GVCFWriter to allow push/pull iteration. #5311
Conversation
import static htsjdk.variant.vcf.VCFConstants.MAX_GENOTYPE_QUAL; | ||
import static org.broadinstitute.hellbender.utils.variant.writers.GVCFWriter.GVCF_BLOCK; | ||
|
||
public final class GVCFBlockCombiner implements PushPullTransformer<VariantContext> { |
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.
Laura made changes to the gvcf writer which I think we'll have to update here.. since I wrote this way before then.
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.
I rebased to take care of these changes (from #4940) - there were quite a few manual changes needed.
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.
Ah, that makes sense since otherwise the tests should be exploding. I just wanted to be sure about it...
Codecov Report
@@ Coverage Diff @@
## master #5311 +/- ##
===============================================
+ Coverage 86.984% 86.988% +0.004%
- Complexity 31208 31224 +16
===============================================
Files 1909 1914 +5
Lines 144202 144264 +62
Branches 15954 15956 +2
===============================================
+ Hits 125433 125492 +59
- Misses 12999 13003 +4
+ Partials 5770 5769 -1
|
c17ea51
to
3b40d8e
Compare
3b40d8e
to
1489950
Compare
@tomwhite Apologies for not getting around to this one before leaving, but rather than keep it blocked while I'm away I'm re-assigning to @cmnbroad, who I'm sure can give you a speedy review. |
1489950
to
9ba41d9
Compare
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.
A few comments and questions. Also, although ideally GVCFBlockCombiner
would have separate unit tests, it appears to have 100% coverage through GVCFWriterUnitTest
. Finally, @ldgauthier FWIW note this is going to create conflicts with your changes in #5312.
src/main/java/org/broadinstitute/hellbender/utils/iterators/PushToPullIterator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/iterators/PushToPullIterator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/iterators/PushToPullIterator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/iterators/PushToPullIterator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/iterators/PushToPullIterator.java
Outdated
Show resolved
Hide resolved
src/testUtils/java/org/broadinstitute/hellbender/testutils/VariantContextTestUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/variant/writers/HomRefBlock.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/variant/writers/GVCFBlockMergingIterator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/variant/writers/GVCFWriter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/iterators/PushToPullIterator.java
Show resolved
Hide resolved
9ba41d9
to
49fcc40
Compare
Thanks for your review @cmnbroad. I've addressed all your points. Please approve so this can be merged. |
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 now thx @tomwhite.
Thank you @cmnbroad! |
This is @lbergelson's work from https://github.com/broadinstitute/gatk/compare/lb_updates_for_haplotype_caller, rebased for master. It is needed for #5138.