Fixed a bug in ReblockGVCF that could cause the first position on a contig to be dropped.

[samuelklee]

Closes #7884.

The bug was caused by two position-only checks that should have been contig+position checks.

I added a minimal regression test that uses a 2-record snippet of the test data discussed in the first post of the issue.

Also discussed there is expected behavior that this fix does not induce. It seems the expectation is that a QUAL = 0 variant block should be turned into a GQ0 reference block.

However, I wonder if this is not actually representative of the current (pre-fix) behavior of the tool. For example, if the test data is run only over chr13 using master, the position is not dropped (since then the position-only checks are valid). In that case, we do not see a GQ0 reference block; we instead see a GQ40 reference block, since the original record had GQ61.

With the fix, we reproduce this reference block. So although we do not induce the expected GQ0 behavior, I would say the bug is fixed.

Whether or not we should issue an additional fix to induce the expected GQ0 behavior is another question entirely. I'm not completely sure what the expected behavior should be from the tool or code documentation alone. Someone more familiar with this tool (@droazen perhaps you can suggest?) may have to chime in. They should probably also check that the contig+position checks are all that need to be added to address the original bug; I'm not 100% sure about behavior there either, but at least all other integration tests seem to still pass.

[codecov]

Codecov Report

Merging #8028 (c778866) into master (ef50c08) will not change coverage.
The diff coverage is 94.737%.

❗ Current head c778866 differs from pull request most recent head 50159a5. Consider uploading reports for the commit 50159a5 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##              master     #8028   +/-   ##
===========================================
  Coverage     86.659%   86.659%           
- Complexity     38881     38888    +7     
===========================================
  Files           2333      2333           
  Lines         182340    182355   +15     
  Branches       20022     20021    -1     
===========================================
+ Hits          158014    158027   +13     
- Misses         17300     17301    +1     
- Partials        7026      7027    +1     

Impacted Files	Coverage Δ
...bender/tools/walkers/variantutils/ReblockGVCF.java	80.244% <75.000%> (ø)
...lkers/variantutils/ReblockGVCFIntegrationTest.java	97.993% <100.000%> (+0.106%)	⬆️
.../hellbender/utils/python/PythonUnitTestRunner.java	75.410% <0.000%> (-3.279%)	⬇️

[tmelman]

Ok to merge this.

Commenting that the implementation of replaced GQ is in line with the expected behavior. The issue claims that the expected behavior is that QUAL 0 variant sites get replaced with GQ 0 hom var sites. This is true; but QUAL 0 hom ref sites will have their original GQ rebinned but not replaced with 0, as expected.

[droazen]

Thanks for fixing this @samuelklee ! For the GQ0 question I would consult with Laura when she returns, as she is the sole maintainer of this tool.