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

Adds support for reading CSI indexes. #998

Merged
merged 3 commits into from
Jan 4, 2021

Conversation

valeriuo
Copy link
Contributor

@valeriuo valeriuo commented Nov 22, 2017

Description

Handles the CSI indexes implemented in HTSJDK. More specifically, it uses the new type, BAM_CSI_TYPE.


Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

@gbggrant
Copy link
Contributor

retest this please

@gbggrant gbggrant self-requested a review January 17, 2018 16:11
@gbggrant gbggrant self-assigned this Jan 17, 2018
@valeriuo
Copy link
Contributor Author

@gbggrant if you are referring to the failed Travis build, the symbol SamReader.Type.BAM_CSI_TYPE is defined in the htsjdk PR 1040, which is still under review. So, I am assuming we'll need to wait for that PR to be merged in order to rebuild this branch.
In the meantime, if there are other tests you need me to perform with my own version of htsjdk, which contains the missing symbol, please let me know.

@magicDGS
Copy link
Contributor

@valeriuo - you can use a htsjdk snapshot from your PR (if I am correct) from the Broad artifactory, which is included in Picard.

Like that, you can test that it is working and hold the merge until a released version with your fix is in.

@yfarjoun
Copy link
Contributor

yfarjoun commented Apr 8, 2019

the dependent PR was merged into master and released in htsjdk 2.19.0, so if you rebase this PR on picard/master the tests should pass.

Copy link
Contributor

@yfarjoun yfarjoun left a comment

Choose a reason for hiding this comment

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

needs tests...

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 81.635% when pulling fa38dcd on valeriuo:feature/vo2_csi into 5843068 on broadinstitute:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 81.635% when pulling fa38dcd on valeriuo:feature/vo2_csi into 5843068 on broadinstitute:master.

Copy link
Contributor

@gbggrant gbggrant left a comment

Choose a reason for hiding this comment

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

@valeriuo looks good, but you are checking in some large files (> 1MB) for tests. Could you please reduce the size of these?
Ideally, we prefer a strategy where simple, documented sam files are checked in, and then the test creates bams on the fly for testing.
If this is not possible, could you try to re-use existing checked in bams and indices for tests?

@FredericBGA
Copy link

Hi,
What is missing for this PR to be merged? Can I help?
We are really interested in this new feature, so I am interested if I can do something.

@valeriuo
Copy link
Contributor Author

valeriuo commented Apr 3, 2020

I will have another look at this. The rebase went smoothly, but it probably needs a few changes, as I see some stuff has been deprecated in the meantime.

@yfarjoun
Copy link
Contributor

@gbggrant it seems that the PR has been updated in response to your comments would you be able to take another look and move it along?

Copy link
Contributor

@gbggrant gbggrant left a comment

Choose a reason for hiding this comment

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

Looks good.

@gbggrant gbggrant merged commit e976c2a into broadinstitute:master Jan 4, 2021
@valeriuo valeriuo deleted the feature/vo2_csi branch January 4, 2021 16:24
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.

6 participants