[codex] Polish RankQuant maintenance surfaces#268
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fa2395218
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
/agentic_review |
Looking for bugs?Check back in a few minutes. Qodo's review agents are on it. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Review Summary
Overall this is a well-structured maintenance PR. The bench-utils feature gating is clean, the API changes are correct, and the comment trimming in rank_to_bucket reduces noise without sacrificing clarity.
One outstanding concern from the previous review (P2) remains partially unaddressed — the SDE AVX-512 jobs still don't enable bench-utils, so the SIMD-parity tests are skipped under SDE. The parity tests are valuable for verifying that AVX-512 produces identical results to the scalar reference, and they should run in that environment.
P2: SDE jobs missing --features bench-utils
The avx512 job in .github/workflows/ci.yml:437-447 and release-avx512 in .github/workflows/release.yml:254-261 run:
cargo test
cargo test --features experimentalThe SIMD-parity tests (rt2_asym_*_matches_scalar, byte_lut_huge_k_*, batched_subset_rerank_matches_scalar_reference_across_tiers, delta_e1_byte_lut_panics_on_b1_index) are all gated #[cfg(feature = "bench-utils")] and won't run without that feature flag. Adding --features bench-utils to both cargo test commands is a one-line change per command and ensures the AVX-512 kernels are verified against the byte-LUT scalar reference under SDE.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/Project-Navi/ordvec/actions/runs/27857100213
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
BucketCode/RankQuantSpecwith the current fixed-composition b=8 contract: b=8 is accepted whendim % 256 == 0and rejected as non-uniform otherwise.search_asymmetric_byte_luthelper behind a non-defaultbench-utilsfeature, and update the benchmark/docs/tests that intentionally use it.checked_new_lentochecked_new_count, addL2_NORMALISE_EPSILON, pin L2 threshold-edge behavior, and trim dense inline rationale comments in rank bucketing helpers.Validation
cargo test --lockedcargo test --locked --all-featurescargo clippy --locked --all-targets --all-features -- -D warningscargo check --locked --features bench-utils --example bench_rankcargo fmt --checkgit diff --check