GenomicsDB matches CombineGVCFs with input spanning deletions #5397

kgururaj · 2018-11-08T20:10:15Z

The newest release of GenomicsDB treats spanning deletions (spanning
from earlier positions) as deletions in the min PL value computation.
This behavior now matches the behavior of CombineGVCFs.

A more detailed description of the issue is provided in
#4963

Deleted a couple of files which are no longer necessary.
Fixed the index of newMQcalc.combined.g.vcf
Fixes GenomicsDBImport: A fatal error has been detected by the Java Runtime Environment #5045 (error out with a helpful error message)
Fixes Possible race condition in GenomicsDBImport multi-interval mode. #5300

kgururaj · 2018-11-08T20:11:01Z

@nalinigans - this should fix the tests with respect to the latest jar

kgururaj · 2018-11-08T20:16:42Z

This release also errors out with a descriptive error message if the length of a field in the data lines does not match the length descriptor in the header - see #5045.
Error out behavior as per Laura's comment here

codecov-io · 2018-11-08T21:10:30Z

Codecov Report

Merging #5397 into master will decrease coverage by 6.656%.
The diff coverage is 76.923%.

@@              Coverage Diff               @@
##             master     #5397       +/-   ##
==============================================
- Coverage     87.05%   80.394%   -6.656%     
+ Complexity    31480     29890     -1590     
==============================================
  Files          1923      1923               
  Lines        145148    145155        +7     
  Branches      16081     16082        +1     
==============================================
- Hits         126352    116696     -9656     
- Misses        12944     22806     +9862     
+ Partials       5852      5653      -199

Impacted Files	Coverage Δ	Complexity Δ
...ls/genomicsdb/GenomicsDBImportIntegrationTest.java	`88.47% <100%> (+0.103%)`	`82 <2> (+2)`	⬆️
.../hellbender/tools/genomicsdb/GenomicsDBImport.java	`78.195% <50%> (-0.892%)`	`53 <0> (ø)`
...rs/variantutils/SelectVariantsIntegrationTest.java	`0.255% <0%> (-99.745%)`	`1% <0%> (-70%)`
...kers/filters/VariantFiltrationIntegrationTest.java	`0.826% <0%> (-99.174%)`	`1% <0%> (-25%)`
...dorientation/CollectF1R2CountsIntegrationTest.java	`0.917% <0%> (-99.083%)`	`1% <0%> (-12%)`
.../walkers/bqsr/BaseRecalibratorIntegrationTest.java	`1.031% <0%> (-98.969%)`	`1% <0%> (-7%)`
...ers/vqsr/FilterVariantTranchesIntegrationTest.java	`1.053% <0%> (-98.947%)`	`1% <0%> (-5%)`
...s/variantutils/VariantsToTableIntegrationTest.java	`1.205% <0%> (-98.795%)`	`1% <0%> (-20%)`
...walkers/validation/ConcordanceIntegrationTest.java	`1.563% <0%> (-98.438%)`	`1% <0%> (-5%)`
...ientation/ReadOrientationModelIntegrationTest.java	`1.667% <0%> (-98.333%)`	`1% <0%> (-5%)`
... and 154 more

nalinigans

Looks good.

kgururaj · 2018-11-16T19:33:23Z

src/main/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBImport.java

            final SimpleInterval queryInterval = new SimpleInterval(chr, start, end);
            if( !interval.equals(queryInterval)){
-                throw new GATKException("Cannot call query with different interval, expected:" + this.interval + " queried with: " + queryInterval);
+                this.interval = new SimpleInterval(chr, start, end);
+                this.query = reader.query(interval.getContig(), interval.getStart(), interval.getEnd());


@droazen @lbergelson would it be possible for one of you take a look at this fix for #5300? I think my fix preserves the original intent of InitializedQueryWrapper which appears to be to speed up the opening of VCF readers. I didn't write this part of the code so I may not fully understand the objective.

droazen · 2018-12-10T18:07:20Z

@kgururaj Could you rebase to resolve conflicts on this branch?

droazen

Added my comments @kgururaj -- back to you.

src/main/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBImport.java

droazen · 2019-01-03T19:52:19Z

@kgururaj Pinging you on this one -- once the review comments above are addressed and the branch is rebased, I think this is good to go.

droazen · 2019-01-07T18:48:15Z

Since @kgururaj is away, I'll address the remaining comments on this PR myself, since they are so minor.

from earlier positions) as deletions in the min PL value computation. This behavior now matches the behavior of CombineGVCFs. A more detailed description of the issue is provided in #4963 * Deleted a couple of files which are no longer necessary. * Fixed the index of newMQcalc.combined.g.vcf

reader-threads are used in the importer. Not a race condition in GenomicsDB - InitializedQueryWrapper wasn't written for multiple intervals. CI test with multiple reader threads

droazen · 2019-01-09T18:10:48Z

@nalinigans @kgururaj I addressed the review comments and rebased the branch, however after rebasing some of the GenomicsDB tests that assert an exact match against CombineGVCFs output are failing, despite my regenerating the expected output using the latest CombineGVCFs.

See the commit 500b026, where I've added comments to the failing tests testGenomicsDBImportFileInputs_newMQ() and testGenomicsDBImportFileInputsAgainstCombineGVCFWithNonDiploidData()

Could one of you take a look at these failures and give your opinion? Is it possible that GenomicsDB no longer matches CombineGVCFs in these cases?

I found a note in the recent commit fb70191 (from the PR #5471) in which the author found it necessary to make separate copies of the HaplotypeCaller expected GVCFs in order to get the GenomicsDB tests to pass. This is likely related to the test failures we're seeing now.

droazen · 2019-01-09T18:52:35Z

Could also possibly be related to #5160?

droazen · 2019-01-09T21:50:04Z

Hmm, that's strange, now only testGenomicsDBImportFileInputs_newMQ() is failing. testGenomicsDBImportFileInputsAgainstCombineGVCFWithNonDiploidData(), which was failing for me this morning, is now passing despite no additional changes.

Error is "Attribute RAW_MQandDP expected [353426] but found [352585]". See https://storage.googleapis.com/hellbender-test-logs/build_reports/master_24137.4/tests/test/index.html

nalinigans · 2019-01-14T02:29:22Z

@droazen, looks like the changes from PR #5540 caused the failure with testGenomicsDBImportFileInputs_newMQ. Reverting the changes from that pull request got things working again. I will look at this issue tomorrow, meanwhile any suggestions @ldgauthier?

Got the tests to pass by generating new expected results for the following failing tests and use the GenomicsDBImports folder for expected results as that was updated as part of PR 5170. * testGenomicsDBImportFileInputsAgainstCombineGVCFWithNonDiploidData * testGenomicsDBImportFileInputs_newMQ Note : Merged with master.

…s_match

nalinigans · 2019-01-16T02:06:51Z

Got the tests to pass by generating a new expected result as mentioned in @kgururaj comment for expected.testGenomicsDBImportWithNonDiploidData.vcf and by using the GenomicsDBImports folder for expected results as some of the vcf files were updated as part of PR 5170.

testGenomicsDBImportFileInputsAgainstCombineGVCFWithNonDiploidData
testGenomicsDBImportFileInputs_newMQ

@droazen, please feel free to merge. Thanks.

kgururaj requested review from droazen, lbergelson and nalinigans November 8, 2018 20:10

kgururaj added the GenomicsDB label Nov 8, 2018

nalinigans approved these changes Nov 15, 2018

View reviewed changes

nalinigans assigned kgururaj Nov 15, 2018

nalinigans mentioned this pull request Nov 16, 2018

Migrate to genomicsdb.org fork of GenomicsDB #5277

Closed

kgururaj force-pushed the genomicsdb_combine_gvcfs_spanning_deletions_match branch from 3a56003 to 8ad1991 Compare November 16, 2018 19:27

kgururaj commented Nov 16, 2018

View reviewed changes

kgururaj force-pushed the genomicsdb_combine_gvcfs_spanning_deletions_match branch from 8ad1991 to e9f5da7 Compare November 19, 2018 18:25

droazen suggested changes Dec 10, 2018

View reviewed changes

src/main/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBImport.java Outdated Show resolved Hide resolved

src/main/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBImport.java Show resolved Hide resolved

droazen assigned droazen and unassigned kgururaj Jan 7, 2019

kgururaj and others added 4 commits January 8, 2019 15:29

Fix for #5300 when multiple

eb5d4b9

reader-threads are used in the importer. Not a race condition in GenomicsDB - InitializedQueryWrapper wasn't written for multiple intervals. CI test with multiple reader threads

Address review comments

a78de0c

Try to fix failing tests

500b026

droazen force-pushed the genomicsdb_combine_gvcfs_spanning_deletions_match branch from e9f5da7 to 500b026 Compare January 9, 2019 18:03

droazen assigned nalinigans Jan 9, 2019

nalinigans mentioned this pull request Jan 15, 2019

Get tests to pass for Issue 5397 #5581

Merged

nalinigans added 2 commits January 15, 2019 16:32

Merge branch 'master' into genomicsdb_combine_gvcfs_spanning_deletion…

b98f9d4

…s_match

droazen approved these changes Jan 16, 2019

View reviewed changes

droazen merged commit 05aa6b2 into master Jan 16, 2019

droazen deleted the genomicsdb_combine_gvcfs_spanning_deletions_match branch January 16, 2019 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GenomicsDB matches CombineGVCFs with input spanning deletions #5397

GenomicsDB matches CombineGVCFs with input spanning deletions #5397

kgururaj commented Nov 8, 2018 •

edited

Loading

kgururaj commented Nov 8, 2018

kgururaj commented Nov 8, 2018

codecov-io commented Nov 8, 2018 •

edited

Loading

nalinigans left a comment

kgururaj Nov 16, 2018

droazen commented Dec 10, 2018

droazen left a comment

droazen commented Jan 3, 2019

droazen commented Jan 7, 2019

droazen commented Jan 9, 2019

droazen commented Jan 9, 2019

droazen commented Jan 9, 2019 •

edited

Loading

nalinigans commented Jan 14, 2019 •

edited

Loading

nalinigans commented Jan 16, 2019 •

edited

Loading

GenomicsDB matches CombineGVCFs with input spanning deletions #5397

GenomicsDB matches CombineGVCFs with input spanning deletions #5397

Conversation

kgururaj commented Nov 8, 2018 • edited Loading

kgururaj commented Nov 8, 2018

kgururaj commented Nov 8, 2018

codecov-io commented Nov 8, 2018 • edited Loading

Codecov Report

nalinigans left a comment

Choose a reason for hiding this comment

kgururaj Nov 16, 2018

Choose a reason for hiding this comment

droazen commented Dec 10, 2018

droazen left a comment

Choose a reason for hiding this comment

droazen commented Jan 3, 2019

droazen commented Jan 7, 2019

droazen commented Jan 9, 2019

droazen commented Jan 9, 2019

droazen commented Jan 9, 2019 • edited Loading

nalinigans commented Jan 14, 2019 • edited Loading

nalinigans commented Jan 16, 2019 • edited Loading

kgururaj commented Nov 8, 2018 •

edited

Loading

codecov-io commented Nov 8, 2018 •

edited

Loading

droazen commented Jan 9, 2019 •

edited

Loading

nalinigans commented Jan 14, 2019 •

edited

Loading

nalinigans commented Jan 16, 2019 •

edited

Loading