Repository navigation
Build the CSI index of an existing BGZF BCF through IndexFactory - #1937
Merged
Merged
Conversation
htsjdk wrote a BCF's CSI only while writing it, with INDEX_ON_THE_FLY,
and IndexFactory's CSI path went through TabixIndexCreator, which needs
a text codec, so a BCF written without the option or by bcftools could
not be indexed ("Binary codecs does not support tabix"). IndexFactory
now builds a BGZF BCF's CSI, recognising the BCF by its contents since
callers may wrap the codec: BCFFileReader.createCsiIndex() reads the
records and builds the index BCF2Writer builds while writing, and the
new BCFCsiIndex writes it as the bare CSI bcftools index writes. TABIX
is refused for a BCF, as htslib refuses it, and CSI for a raw BCF. A
file that cannot be read still fails as the other formats' indexer
reports it, with a TribbleException.
The reader and the writer now start the index the same way, through
BCFCsiIndex.binningIndexBuilderFor, which gives a header whose contig
lines carry no lengths htslib's reach of 2^31 - 1 bases rather than
2^17, and both index a record at POS 0 at 1, as htslib does; either
used to fail. getFeatureReader refuses a BCFCsiIndex given in memory,
whose virtual offsets TribbleIndexedFeatureReader would read as file
offsets and so find nothing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 12 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: building a CSI index for an existing BGZF-compressed BCF through IndexFactory. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 12 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
getFeatureReader(resource, codec, Index) always builds a TribbleIndexedFeatureReader, which takes an index's blocks for file offsets. A TabixIndex's blocks, like a BCFCsiIndex's, are BGZF virtual offsets, so a query of a bgzipped VCF through a TBI given this way fed compressed bytes to the codec and failed with a parse error. Both are now refused with a message saying to open the file with the index's path, which the other getFeatureReader overloads route to the right reader.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IndexFactory.createIndex(path, codec, IndexType.CSI, dictionary)now builds the CSI index of an existing BGZF-compressed BCF, so a BCF written withoutINDEX_ON_THE_FLY, or by bcftools, can be indexed without bcftools.IndexFactory's CSI path needs a text codec and failed with "Binary codecs does not support tabix" (found by GATK's IndexFeatureFile tests on 6.0).IndexFactoryrecognises a BCF by its contents, since callers may wrap the codec.BCFFileReader.createCsiIndex()reads the records into the index the writer builds on the fly. The newBCFCsiIndexwrites it as the bare CSIbcftools indexwrites.IndexType.TABIXis refused for a BCF, as htslib refuses it.IndexType.CSIis refused for a BCF that is not BGZF-compressed. A file that can't be read still fails with aTribbleException, as before.BCFCsiIndex.binningIndexBuilderFor(VCFHeader). This fixes two bugs in on-the-fly indexing, which is new in 6.0. Both failed before; both now match bcftools:length=got an index reaching only 131,072 bases. It now reaches 2^31 - 1, as htslib does.getFeatureReader(resource, codec, Index)now refuses a TBI or CSI index given in memory (TabixIndexorBCFCsiIndex). It always builds aTribbleIndexedFeatureReader, which reads their BGZF virtual offsets as file offsets. A BCF query then found nothing, and a bgzipped VCF query through an in-memory TBI failed with a parse error on compressed bytes. The message points to the overloads that take an index path, which pick the right reader. Nothing in GATK, Picard or IGV uses this overload.bcftools indexnow also nameIndexFactory.IndexFactory.loadIndexon a bare CSI says that such an index is read through the file it indexes.bcftools index --csi(1.24): the indexes are equal (BinningIndex.equals). Inputs: htsjdk-written BCFs with block-aligned records, long END/SVLEN records, empty and sparse contigs, 200 KB records, contigs without lengths, POS 0, and bcftools' ownvqsr.bcf(131,821 records, 0.9 s). Tests compare with bcftools when it is installed.BinningIndex.Builder, which BAM, CRAM and tabix share.n_refdiffers from bcftools'. Queries are unaffected.Reviewed with Sonnet and Opus.
Summary by CodeRabbit