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

feat: Index for bam::IndexedReader #387

Merged
merged 20 commits into from
May 12, 2023
Merged

Conversation

tedil
Copy link
Member

@tedil tedil commented May 2, 2023

This PR addresses issue #386 by providing a struct IndexView analogous to HeaderView.

Since I'm not particularly well versed in interfacing with C and the htslib API, I'm a bit unsure if this is "the correct way" to handle this. There's no corresponding implementation for Clone yet, and I'm not sure what the point of the owned field of HeaderView is, since it does not seem to be modified anywhere.

This still needs some testcases, a simple one should be the one from the referenced issue.

@coveralls
Copy link

coveralls commented May 2, 2023

Pull Request Test Coverage Report for Build 4946873208

  • 64 of 77 (83.12%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.3%) to 79.293%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/bam/mod.rs 64 77 83.12%
Files with Coverage Reduction New Missed Lines %
src/utils.rs 1 66.67%
src/bam/record.rs 2 76.33%
Totals Coverage Status
Change from base Build 4698271908: 0.3%
Covered Lines: 2332
Relevant Lines: 2941

💛 - Coveralls

@tedil tedil changed the title Index for indexed reader feat: Index for indexed reader May 2, 2023
@tedil tedil changed the title feat: Index for indexed reader feat: Index for bam::IndexedReader May 2, 2023
@tedil tedil marked this pull request as ready for review May 2, 2023 13:34
@tedil
Copy link
Member Author

tedil commented May 2, 2023

This currently only works for BAM + bai, not CRAM + crai, unsure why that is.

@tedil
Copy link
Member Author

tedil commented May 5, 2023

Ah, the cram index does not contain the necessary information, so the slow idxstats variant has to be used, see bam_index.c for more information

@tedil tedil marked this pull request as draft May 5, 2023 09:48
@tedil
Copy link
Member Author

tedil commented May 5, 2023

I've added an unpolished translation of the slow_idxstats method from samtools bam_index.c, which should be used in case of CRAM (and SAM) indexed files.

@tedil tedil marked this pull request as ready for review May 11, 2023 10:39
@johanneskoester
Copy link
Contributor

Nice work! If I remember correctly, the owned flag was for destroying the header view, which is not always what you want. But I don't recall the details anymore and don't have the time to check the code. However, this one looks good to me for now.

@johanneskoester johanneskoester merged commit fb74387 into master May 12, 2023
@johanneskoester johanneskoester deleted the index-for-indexed-reader branch May 12, 2023 12:32
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.

3 participants