Fixed 1097 bug where GL field overrides PL field. #1098
Conversation
…e inconsistent, and GL is "after" PL, the resulting genotype has the wrong PL value. - added test which failed prior to the change.
|
to the reviewer: use |
| Assert.assertEquals(new VCF3Codec().getTabixFormat(), TabixFormat.VCF); | ||
| } | ||
|
|
||
| private void assertIntArraysAreEqual(int[] v1, int[] v2) { |
There was a problem hiding this comment.
I think that TestNG has already this method (https://github.com/cbeust/testng/blob/master/src/main/java/org/testng/Assert.java#L238)
There was a problem hiding this comment.
I couldn't find that in the version that we have in htsjdk....I could rev TestNG I guess.
There was a problem hiding this comment.
Fair enough. I just didn't know in which version it was introduced...
Codecov Report
@@ Coverage Diff @@
## master #1098 +/- ##
==============================================
+ Coverage 66.185% 66.515% +0.33%
- Complexity 7636 7748 +112
==============================================
Files 535 535
Lines 32403 32743 +340
Branches 5513 5627 +114
==============================================
+ Hits 21446 21779 +333
- Misses 8792 8800 +8
+ Partials 2165 2164 -1
|
- removed unneeded helper function
There was a problem hiding this comment.
@yfarjoun Thanks for the fix. I think we should probably expose a getter instead of call to make, although that might mean we need to expose getters for the other built in attributes for symmetry.
Should we be in the business of detecting gl /pl disagreement and warning about it? Or do we just not care because gl is deprecated?
| gb.PL(GenotypeLikelihoods.fromGLField(genotypeValues.get(i)).getAsPLs()); | ||
| // Do not overwrite PL with data from GL | ||
| if (!gb.make().hasPL()) { | ||
| gb.PL(GenotypeLikelihoods.fromGLField(genotypeValues.get(i)).getAsPLs()); |
There was a problem hiding this comment.
@yfarjoun Thanks for fixing this. I don't love the call to make() in this if statement. It would be nice to avoid any extra allocations. It might be worth adding a getPL method to the builder in order to avoid having to do that.
There was a problem hiding this comment.
If this were needed in production code, I'd agree with you, but given that I'm using it in a test, I hesitate to add this feature when it didn't seem to be needed thus far.
| public void testGLnotOverridePL() { | ||
| VCFFileReader reader = new VCFFileReader(new File("src/test/resources/htsjdk/variant/test_withGLandPL.vcf"), false); | ||
| VariantContext variant = reader.iterator().next(); | ||
| reader.close(); |
There was a problem hiding this comment.
always better to do this in a try-with-resources instead of the manual call to close
There was a problem hiding this comment.
right...i just copied from above...changing in both places.
|
I don't want to be detecting PL/GL disagreements, but I think that overwriting the PL with a transformed GL is wrong...this is what I'm protecting against. |
|
bump |
|
@yfarjoun Push is failing because it's out of date, but PR is good, so I'm merging this. Thank you. |
When GL and PL are inconsistent, and GL is "after" PL, the resulting genotype has the wrong PL value.
fixes #1097