Repository navigation
Remove the NumBits and NumVals traits - #130
Merged
Merged
Conversation
Both traits only reported size statistics and had almost no generic users. `NumVals` was never used as a bound, and `NumBits` was only used by `WaveletMatrix`, which now stores the number of zeros in each layer and the sequence length itself. - Remove `bit_vectors::NumBits` and `int_vectors::NumVals` with their impls and prelude re-exports. - Add inherent `num_ones()` and `num_zeros()` to `BitVector`, `Rank9Sel`, `DArray`, and `SArray`. - Store `zeros` and `len` in `WaveletMatrix` and serialize them, so its layer bound is now `Access + Rank + Select`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Serialize `zeros` and `len` after `max_value`, so that reading data in the previous format fails with an EOF error instead of interpreting `max_value` as the length of `zeros` and attempting a huge allocation. - Return `InvalidState` if `zeros.len()` differs from `layers.len()`, since zipping them would otherwise silently return wrong values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The old format is not something we keep maintaining. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Summary
bit_vectors::NumBitsandint_vectors::NumVals, along with their impls andpreludere-exports.num_ones()andnum_zeros()toBitVector,Rank9Sel,DArray, andSArray. Lengths are available through the existing inherentlen()/is_empty().WaveletMatrixnow stores the number of zeros in each layer (zeros) and the sequence length (len), so its layer bound becomesAccess + Rank + Select.Why
Both traits only reported size statistics and were an over-abstraction, in the same spirit as #129:
NumValshad no generic users at all. Every impl just forwarded to the inherentlen().NumBits::num_bitsalso just forwarded tolen(). The trait name described only one of its three methods.num_oneswas not part of the contract: O(1) forRank9Sel,DArray, andSArray, but O(u) forBitVector.WaveletMatrix, which callednum_zeros()on every layer in its query loops. These values are known at construction time, so storing them is the textbook layout (Z_l in Claude and Navarro's paper). It also avoids the O(u) scan per query whenBitVectoris used as a layer.We also considered keeping a minimal length-only trait (
Len) for future generic structures such as LOUDS or FM-index, but decided against it: such structures can store the values they need in the same way asWaveletMatrix.Notes for reviewers
NumBits/NumValsin imports or bounds, or callsnum_bits()/num_vals(), must switch tolen(). Calls tonum_ones()/num_zeros()on concrete types keep working since they are now inherent methods. This should go into 0.10.0.WaveletMatrix:zerosandlenare now serialized afterlayersandmax_value, so data serialized by earlier versions cannot be read.InvalidStateifzeros.len()differs fromlayers.len(), since zipping them would otherwise silently return wrong values (test_deserialize_inconsistent_zeros).zeroscould be recomputed at load time asrank0(len)if onlylenwere stored, but that would require widening theSerializablebound toB: Serializable + Rank, cost O(u) per layer forBitVectorlayers, and panic at load time forDArraylayers withoutenable_rank(). Storingzeroskeeps the bound asB: Serializable. The overhead is 8 bytes per layer plus 8 bytes.EliasFano::from_bitsnow usesbv.is_empty()instead of comparing the length to zero (clippylen_zero).Test plan
cargo test --workspacecargo test --no-default-featurescargo clippy --workspace --all-targets -- -D warningscargo clippy --no-default-features -- -D warningsRUSTDOCFLAGS="-D warnings" cargo doc --no-depscargo fmt --all -- --checkcargo +1.62.0 check --lib(MSRV)🤖 Generated with Claude Code