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

updating htsjdk to 2.18.1 #5486

Merged
merged 1 commit into from
Dec 5, 2018
Merged

updating htsjdk to 2.18.1 #5486

merged 1 commit into from
Dec 5, 2018

Conversation

lbergelson
Copy link
Member

updating htjsdk 2.18.0 -> 2.18.1
this fixes an issue that prevents gatk from reading certain cram files

updating htjsdk 2.18.0 -> 2.18.1
this fixes an issue that prevents gatk from reading certain cram files
@lbergelson lbergelson requested a review from cmnbroad December 5, 2018 17:40
@droazen droazen requested review from droazen and removed request for cmnbroad December 5, 2018 18:34
@droazen droazen self-assigned this Dec 5, 2018
Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

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

LGTM once tests finish.

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.

One comment to address, then good to merge 👍

@@ -57,7 +56,7 @@ repositories {
}

final requiredJavaVersion = "8"
final htsjdkVersion = System.getProperty('htsjdk.version','2.18.0')
final htsjdkVersion = System.getProperty('htsjdk.version','2.18.1')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an integration test with one of the CRAMs in question that fails with 2.18.0 and passes with 2.18.1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Joel tried to build one and failed....

Copy link
Contributor

Choose a reason for hiding this comment

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

I think he might have figured it out -- @jmthibault79 can you comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

@cmnbroad cmnbroad Dec 5, 2018

Choose a reason for hiding this comment

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

I think it would be hard to generate one of these with htsjdk even if we had a fix for samtools/htsjdk#1236, since htsjdk doesn't use that (BetaEncoding) for alignment start; it uses ExternalIntegerEncoding. Its probably samtools that uses (or once used) that encoding for alignment start.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, fair enough -- we can merge this as-is then.

@droazen droazen assigned lbergelson and unassigned droazen Dec 5, 2018
Copy link
Contributor

@jmthibault79 jmthibault79 left a comment

Choose a reason for hiding this comment

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

Can read a NA12878.cram with -L UNMAPPED with this change; crashes on master.

@droazen droazen merged commit 637f4fa into master Dec 5, 2018
@droazen droazen deleted the lb_update_htsjdk branch December 5, 2018 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants