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.

Sign up for GitHub

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

Jump to bottom

update htsjdk to 2.19.0 #5812

Merged
merged 1 commit into from
Mar 27, 2019
Merged

update htsjdk to 2.19.0 #5812

merged 1 commit into from
Mar 27, 2019

Conversation

lbergelson
Copy link
Member

Changed necessary for htsjdk 2.19.0, but we're blocked by disq-bio/disq#95.

We might also want to wait for broadinstitute/picard#1297 to be incorporated although I think it's ok not to wait.

lbergelson
lbergelson commented Mar 18, 2019
View reviewed changes
build.gradle Outdated
@@ -54,16 +54,19 @@ repositories {
url "https://broadinstitute.jfrog.io/broadinstitute/libs-snapshot/" //for htsjdk snapshots
}

maven {
Copy link
Member Author

Choose a reason for hiding this comment

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

This can go once we update to the new disq.

lbergelson
lbergelson commented Mar 18, 2019
View reviewed changes
src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java
Files.deleteIfExists(path);
}
public static void deleteRecursively(final Path rootPath) {
IOUtil.recursiveDelete(rootPath);
Copy link
Member Author

Choose a reason for hiding this comment

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

New implementation in htsjdk. We might want to just remove this one actually.

lbergelson
lbergelson commented Mar 18, 2019
View reviewed changes
src/test/java/org/broadinstitute/hellbender/tools/IndexFeatureFileIntegrationTest.java
@@ -209,7 +209,8 @@ public void testBCFIndex() {
checkIndex(index, Arrays.asList("1"));
}

@Test(expectedExceptions = UserException.CouldNotIndexFile.class)
// test disabled until https://github.com/samtools/htsjdk/issues/1323 is resolved
@Test(enabled = false)
Copy link
Member Author

Choose a reason for hiding this comment

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

this is resolved but needs yet another htsjdk release to fix.

@lbergelson
Copy link
Member Author

This should add support for reading bam files with CSI indexes, as well as porting the FastaReferenceWriter to htsjdk and a lot of other changes to htsjdk. @samuelklee This includes the overlap detector optimizations you wanted as well as the changes to let you write interval file to paths.

@tomwhite
Copy link
Contributor

Disq 0.3.0 has now been released.

@lbergelson lbergelson marked this pull request as ready for review March 20, 2019 16:16
@lbergelson lbergelson changed the title update htsjdk to 2.19.0 (waiting on disq release) update htsjdk to 2.19.0 Mar 20, 2019
@lbergelson lbergelson requested a review from droazen March 20, 2019 16:22
@codecov-io
Copy link

codecov-io commented Mar 20, 2019 •
edited
Loading

Codecov Report

Merging #5812 into master will decrease coverage by 0.005%.
The diff coverage is 92.308%.

@@              Coverage Diff               @@
##              master    #5812       +/-   ##
==============================================
- Coverage     87.045%   87.04%   -0.005%     
+ Complexity     32198    32059      -139     
==============================================
  Files           1979     1976        -3     
  Lines         147553   147004      -549     
  Branches       16239    16162       -77     
==============================================
- Hits          128437   127952      -485     
+ Misses         13193    13154       -39     
+ Partials        5923     5898       -25
Impacted Files Coverage Δ Complexity Δ
...spark/sv/utils/SingleSequenceReferenceAligner.java 80.822% <ø> (ø) 16 <0> (ø) ⬇️
...lbender/tools/IndexFeatureFileIntegrationTest.java 88.938% <ø> (-2.212%) 32 <0> (-1)
...nder/utils/io/DeleteRecursivelyOnExitPathHook.java 70% <0%> (-5%) 3 <0> (ø)
...gine/spark/datasources/ReadsSparkSinkUnitTest.java 75.61% <100%> (ø) 23 <0> (ø) ⬇️
...r/testutils/testers/MarkDuplicatesSparkTester.java 89.773% <100%> (ø) 22 <0> (ø) ⬇️
...ender/tools/walkers/fasta/FastaReferenceMaker.java 87.234% <100%> (+0.87%) 11 <0> (ø) ⬇️
...nder/tools/walkers/genotyper/GenotypingEngine.java 70.248% <100%> (ø) 72 <0> (ø) ⬇️
...der/tools/spark/CreateHadoopBamSplittingIndex.java 85% <100%> (ø) 14 <0> (ø) ⬇️
...rg/broadinstitute/hellbender/utils/io/IOUtils.java 72.188% <100%> (-0.428%) 97 <1> (-1)
.../CreateHadoopBamSplittingIndexIntegrationTest.java 89.063% <100%> (ø) 13 <0> (ø) ⬇️
... and 11 more

@droazen droazen self-assigned this Mar 25, 2019
droazen
droazen approved these changes Mar 26, 2019
View reviewed changes
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.

Two questions for you @cmnbroad, then feel free to merge (assuming you are satisfied that the Picard dependency issues have been sorted out).

src/main/java/org/broadinstitute/hellbender/tools/walkers/fasta/FastaReferenceMaker.java
@@ -82,7 +83,10 @@
public void onTraversalStart() {
final Path path = IOUtils.getPath(output);
try {
writer = new FastaReferenceWriter(path, basesPerLine, true, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the behavior previously controlled by the two true booleans here being preserved in the new FastaReferenceWriterBuilder call?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like these were makeFai and makeDict, both of which are true by default in the new builder.

src/test/java/org/broadinstitute/hellbender/tools/IndexFeatureFileIntegrationTest.java
@@ -209,7 +209,8 @@ public void testBCFIndex() {
checkIndex(index, Arrays.asList("1"));
}

@Test(expectedExceptions = UserException.CouldNotIndexFile.class)
// test disabled until https://github.com/samtools/htsjdk/issues/1323 is resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a GATK issue to track this, and remind us to re-enable this test in the future?

Copy link
Collaborator

Choose a reason for hiding this comment

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

#5838

@droazen droazen assigned cmnbroad and unassigned droazen Mar 26, 2019
@lbergelson @cmnbroad
Updating htsjdk, disq, and Picard.
8140531 
* Updating htsjdk 2.18.2 -> 2.19.0
* Updating disq 0.2.0 -> 0.3.0

* Disabling test that was relying on broken behavior that was fixed in htsjdk
* Removing code that has migrated to htsjdk
@cmnbroad
Copy link
Collaborator

This also includes an upgrade to Picard 2.19, so its in sync with htsjdk now.

@cmnbroad cmnbroad merged commit b7f90f4 into master Mar 27, 2019
@cmnbroad cmnbroad deleted the lb_update_htsjdk branch March 27, 2019 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmnbroad cmnbroad cmnbroad left review comments

@droazen droazen droazen approved these changes

Assignees

@cmnbroad cmnbroad

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lbergelson @tomwhite @codecov-io @cmnbroad @droazen