Skip to content
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

GenomicsDB Allele-specific Annotation Support and Bug Fixes #4261

Merged

Conversation

psfoley
Copy link
Contributor

@psfoley psfoley commented Jan 25, 2018

This PR adds:

and fixes:

  • 4160
  • 4106
  • 3736
  • 3745
  • Bug fix - Resize LUTs when dealing with spanning deletions

@droazen droazen self-requested a review January 25, 2018 19:30
@droazen droazen self-assigned this Jan 25, 2018
@codecov-io
Copy link

codecov-io commented Jan 25, 2018

Codecov Report

Merging #4261 into master will increase coverage by 0.237%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##              master   #4261       +/-   ##
=============================================
+ Coverage     78.463%   78.7%   +0.237%     
- Complexity     16440   16480       +40     
=============================================
  Files           1039    1040        +1     
  Lines          59173   59338      +165     
  Branches        9686    9706       +20     
=============================================
+ Hits           46429   46699      +270     
+ Misses          8993    8888      -105     
  Partials        3751    3751
Impacted Files Coverage Δ Complexity Δ
...s/spark/ParallelCopyGCSDirectoryIntoHDFSSpark.java 0% <0%> (-75.51%) 0% <0%> (-17%)
...nder/tools/spark/pipelines/PrintVariantsSpark.java 0% <0%> (-66.667%) 0% <0%> (-2%)
...oadinstitute/hellbender/utils/test/XorWrapper.java 13.043% <0%> (-60.87%) 2% <0%> (-6%)
...lbender/engine/datasources/ReferenceAPISource.java 25.735% <0%> (-44.853%) 8% <0%> (-19%)
...utils/smithwaterman/SmithWatermanIntelAligner.java 50% <0%> (-30%) 1% <0%> (-2%)
...oadinstitute/hellbender/utils/gcs/BucketUtils.java 54.194% <0%> (-24.516%) 30% <0%> (-9%)
...nder/tools/spark/BaseRecalibratorSparkSharded.java 0% <0%> (-22.807%) 0% <0%> (-2%)
...ender/engine/datasources/ReferenceMultiSource.java 61.538% <0%> (-15.385%) 9% <0%> (-2%)
...titute/hellbender/utils/test/MiniClusterUtils.java 78.947% <0%> (-10.526%) 6% <0%> (-1%)
...bender/tools/copynumber/CallCopyRatioSegments.java 90.909% <0%> (-9.091%) 2% <0%> (ø)
... and 43 more

@jamesemery
Copy link
Collaborator

jamesemery commented Jan 25, 2018

There is a disabled test in GenomicsDBImportIntegrationTest.testGenomicsDBAlleleSpecificAnnotations() which compares the results to those of combineGVCFs over moderately sized files which you should should try out.

@droazen
Copy link
Contributor

droazen commented Jan 26, 2018

@psfoley Did you just push another bug fix into this branch? Can you explain what the bug was, and create a github ticket with the bug description for our records?

We really need tests for each of the fixes in this branch, as well -- currently there are only tests for the allele-specific annotation support. I may attempt to add some tests later today if time permits.

@droazen droazen changed the title GenomicsDB Allele Annotation Support and Bug Fixes GenomicsDB Allele-specific Annotation Support and Bug Fixes Jan 26, 2018
@droazen
Copy link
Contributor

droazen commented Jan 26, 2018

@psfoley The branch is failing tests after your latest commit with the error:

> Could not resolve all dependencies for configuration ':runtime'.
   > Could not find com.intel:genomicsdb:0.9.2-proto-3.0.0-beta-1+uuid-static.

@psfoley
Copy link
Contributor Author

psfoley commented Jan 26, 2018

@jamesemery and @droazen thank you for reviewing.

@jamesemery I have enabled GenomicsDBImportIntegrationTest.testGenomicsDBAlleleSpecificAnnotations() and this test is passing. Is anything else required?

@droazen I incremented the GenomicsDB release that incorporates a fix for resizing LUTs when dealing with spanning deletions - this was a single line change in GenomicsDB that should fall under the umbrella of enabling allele-specific annotation support 3707. This updated jar has been uploaded, but isn't available on Maven yet (it should be within the next hour).

I agree that additional tests should be added for the bugs that this PR addresses. I'll plan on adding tests for the two high priority fixes (4160 and 3736) this afternoon.

@kgururaj
Copy link
Collaborator

The bug was in an internal structure of GenomicsDB reported by a user

public void testGenomicsDBImportWithoutDBField() throws IOException {
//Test for https://github.com/broadinstitute/gatk/issues/3736
final List<String> vcfInputs = Arrays.asList(NA_24385);
final String workspace = createTempDir("genomicsdb-tests").getAbsolutePath() + "/workspace";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that the indentation in your PRs is always a little off @psfoley -- for future PRs, you may want to look at your IDE settings. We generally set our IDEs to expand tabs into 4 spaces, which may be the issue here.

(Not a blocker for merge of this PR, however)

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@droazen droazen merged commit 385fa4a into broadinstitute:master Jan 29, 2018
public void testLongWorkspacePath() throws IOException {
//Test for https://github.com/broadinstitute/gatk/issues/4160
final List<String> vcfInputs = LOCAL_GVCFS;
final String workspace = createTempDir("long_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa_genomicsdb").getAbsolutePath() + "/should_not_fail_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in person, can you open a separate PR that adds about 150 additional a's to this path @psfoley, and confirm that the test fails if you downgrade the GenomicsDB version in our build.gradle? Thanks!

lbergelson pushed a commit that referenced this pull request Jan 31, 2018
* Incremented GenomicsDB version - Allele annotation support and multiple bug fixes

* Enabled GenomicsDBAlleleSpecificAnnotations Test

* Increment version - Fix for resizing LUTs when dealing with spanning deletions

* Added tests for Issues 4160 and 3736
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants