Repository navigation
test(raw-bam): benchmarks + proptests + edge cases - #281
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #281 +/- ##
=======================================
Coverage 90.02% 90.03%
=======================================
Files 122 122
Lines 61791 61791
=======================================
+ Hits 55629 55631 +2
+ Misses 6162 6160 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds extensive tests and benchmarks to the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/raw_bam_length_changing.rs`:
- Line 1: Add the crate-level attribute to deny unsafe code by inserting
#![deny(unsafe_code)] at the top of the benchmark file (above the existing use
std::hint::black_box;), so the crate enforces the coding guideline prohibiting
unsafe code; ensure the attribute is the very first item in
benches/raw_bam_length_changing.rs before any imports or modules.
In `@benches/raw_bam_tags_editor.rs`:
- Line 1: Add the crate-level attribute to deny unsafe code by inserting
#![deny(unsafe_code)] at the very top of benches/raw_bam_tags_editor.rs (before
any use/imports such as the existing use std::hint::black_box;) so the benchmark
crate enforces the repository rule prohibiting unsafe code.
In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs`:
- Line 1: Add the crate-level attribute to deny unsafe code by inserting the
attribute declaration at the very top of this test file: add the line containing
the attribute `#![deny(unsafe_code)]` above the existing `#![cfg(feature =
"test-utils")]` so the crate forbids unsafe code in this test crate; ensure the
attribute is the first item in the file so it applies to the whole crate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d10acce8-7795-4466-8e9a-79e023f16022
📒 Files selected for processing (7)
Cargo.tomlbenches/raw_bam_accessors.rsbenches/raw_bam_length_changing.rsbenches/raw_bam_tags_editor.rsbenches/raw_bam_vs_recordbuf.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/tests/tags_editor_invariants.rs
47a8eb2 to
64a1553
Compare
6639a7d to
bb34d9d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/fgumi-raw-bam/tests/tags_editor_invariants.rs (1)
186-221: Nit:aux_lenmisnomer.
aux_lencapturesrec.as_ref().len()— the full record length, not aux length. Rename toinitial_record_len(matches the assertion message at L218) to avoid future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs` around lines 186 - 221, The variable aux_len is misleading because it stores the full record length via rec.as_ref().len(), not the auxiliary section length; rename aux_len to initial_record_len (and update its uses in the size assertion and any comments) so the name matches its purpose and the assertion message in the test that checks rec.as_ref().len() >= initial_record_len; update the binding where aux_len is declared and the later prop_assert that references it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/raw_bam_tags_editor.rs`:
- Around line 20-28: The benchmark update_int_resize currently reuses the same
RawRecord (created via RawRecord::from) so only the first iteration forces a
tag-size resize; change the bench to rebuild the record inside each sample by
using Bencher::iter_batched or iter_custom so each iteration constructs a fresh
RawRecord and then calls rec.tags_editor().update_int(b"NM", ...), ensuring the
cost measured is the resize path (reference symbols: update_int_resize benchmark
closure, RawRecord::from, tags_editor(), update_int, and use
iter_batched/iter_custom to recreate the record per sample); also verify
append_then_remove and update_float_in_place intentionally reset state each
iteration and adjust similarly if they should measure resize behavior.
---
Nitpick comments:
In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs`:
- Around line 186-221: The variable aux_len is misleading because it stores the
full record length via rec.as_ref().len(), not the auxiliary section length;
rename aux_len to initial_record_len (and update its uses in the size assertion
and any comments) so the name matches its purpose and the assertion message in
the test that checks rec.as_ref().len() >= initial_record_len; update the
binding where aux_len is declared and the later prop_assert that references it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e57b7c2b-b4a9-4402-9216-4a3ddf09dc8e
📒 Files selected for processing (7)
Cargo.tomlbenches/raw_bam_accessors.rsbenches/raw_bam_length_changing.rsbenches/raw_bam_tags_editor.rsbenches/raw_bam_vs_recordbuf.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/tests/tags_editor_invariants.rs
✅ Files skipped from review due to trivial changes (2)
- benches/raw_bam_accessors.rs
- crates/fgumi-raw-bam/src/raw_bam_record.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- benches/raw_bam_length_changing.rs
- benches/raw_bam_vs_recordbuf.rs
bb34d9d to
92de3a1
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/fgumi-raw-bam/tests/tags_editor_invariants.rs (1)
231-233: Minor: simplifydecode_u8_array.The map-collect over a range is a slow way to write a byte copy.
♻️ Proposed simplification
-fn decode_u8_array(data: &[u8], count: usize) -> Vec<u8> { - (0..count).map(|i| data[i]).collect() -} +fn decode_u8_array(data: &[u8], count: usize) -> Vec<u8> { + data[..count].to_vec() +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs` around lines 231 - 233, The function decode_u8_array is copying bytes inefficiently by mapping over a range; replace the body with a direct slice-to-Vec copy (e.g., use data[..count].to_vec()) in the decode_u8_array function to produce the same Vec<u8> result more efficiently and readably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/raw_bam_vs_recordbuf.rs`:
- Around line 166-180: Update the misleading inline comment in
bench_set_cigar_ops: change the text that says "50M 1D 99M" to "50M 1D 100M"
(the benchmark arrays raw_ops and noodles_ops actually use 50M + 1D + 100M to
sum to l_seq=150), so the comment correctly reflects the values used by raw_ops
and noodles_ops.
---
Nitpick comments:
In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs`:
- Around line 231-233: The function decode_u8_array is copying bytes
inefficiently by mapping over a range; replace the body with a direct
slice-to-Vec copy (e.g., use data[..count].to_vec()) in the decode_u8_array
function to produce the same Vec<u8> result more efficiently and readably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab18f07b-dfd7-482d-90d8-858b42737135
📒 Files selected for processing (7)
Cargo.tomlbenches/raw_bam_accessors.rsbenches/raw_bam_length_changing.rsbenches/raw_bam_tags_editor.rsbenches/raw_bam_vs_recordbuf.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/tests/tags_editor_invariants.rs
✅ Files skipped from review due to trivial changes (3)
- benches/raw_bam_length_changing.rs
- crates/fgumi-raw-bam/src/raw_bam_record.rs
- benches/raw_bam_accessors.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
92de3a1 to
40211c0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/fgumi-raw-bam/tests/tags_editor_invariants.rs (2)
230-261: Nit: simplify the decode helpers.
decode_u8_arrayis justdata[..count].to_vec(); the others could usechunks_exactfor clarity. Not blocking.Optional tightening
-fn decode_u8_array(data: &[u8], count: usize) -> Vec<u8> { - (0..count).map(|i| data[i]).collect() -} +fn decode_u8_array(data: &[u8], count: usize) -> Vec<u8> { + data[..count].to_vec() +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs` around lines 230 - 261, Replace the manual index-based loops in the decode helpers with simpler, safer idioms: in decode_u8_array use data[..count].to_vec() (or data.get(..count).unwrap_or_default() variant if you need bounds safety), and for decode_u16_array, decode_i16_array, decode_i32_array and decode_f32_bits_array use data.chunks_exact(n).take(count).map(|c| <type>::from_le_bytes(c.try_into().unwrap())).collect() (where n is 2 for u16/i16 and 4 for i32/f32 bits) to avoid manual index arithmetic and improve readability; update each function (decode_u8_array, decode_u16_array, decode_i16_array, decode_i32_array, decode_f32_bits_array) accordingly and ensure you handle or assert chunk length if necessary.
360-381: Minor:index % initial.len()biases low indices.When
initial.len()doesn't divide 32, lower indices are slightly more likely. Doesn't affect correctness coverage meaningfully — noting only. Could useprop_flat_maponinitial.len()if you care.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs` around lines 360 - 381, The test set_array_element_u8_writes_one_element currently computes index as index % initial.len(), which biases lower indices; instead generate the index based on the actual initial length by producing the pair (initial, index) together — e.g. use proptest's prop_flat_map (or similar) to first generate initial via proptest::collection::vec(any::<u8>(), 1..32) and then, inside the flat_map closure, generate index from 0..initial.len(), returning (initial, index) so the index distribution is uniform for the actual array length.benches/raw_bam.rs (1)
301-314: Minor:src_view = RawRecordView::new(&src_bytes)inside the timed closure.Hoist it into the setup closure or outside —
RawRecordView::newis cheap but including it in the hot path muddies thecopy_frommeasurement slightly.Suggested tweak
c.bench_function("editor::copy_from", |b| { let src_bytes = multi_tag_bytes(); + let src_view = RawRecordView::new(&src_bytes); b.iter_batched_ref( || { let bytes = make_bam_bytes(0, 0, 0, b"r", &[], 0, -1, -1, &[]); RawRecord::from(bytes) }, |rec| { - let src_view = RawRecordView::new(&src_bytes); rec.tags_editor().copy_from(src_view.tags(), black_box(&[b"NM"])); }, BatchSize::SmallInput, ); });Actually
src_viewborrowssrc_bytes, so lifetimes make the hoist awkward; an alternative is computinglet src_tags_bytes = RawRecordView::new(&src_bytes).tags().as_bytes();once and reconstructing inside. Feel free to skip — noise-level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/raw_bam.rs` around lines 301 - 314, Hoist creation of the source view out of the hot path: avoid calling RawRecordView::new(&src_bytes) inside the timed closure used for measuring tags_editor().copy_from. Instead compute a stable byte-slice once in setup (for example let src_tags_bytes = RawRecordView::new(&src_bytes).tags().as_bytes()) and then inside the iter_batched_ref closure reconstruct a temporary RawRecordView (or otherwise wrap that byte slice) and pass its tags() to rec.tags_editor().copy_from; this keeps RawRecord::from(...) and the measured copy_from call in the hot path while removing the one-time view construction from each iteration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@benches/raw_bam.rs`:
- Around line 301-314: Hoist creation of the source view out of the hot path:
avoid calling RawRecordView::new(&src_bytes) inside the timed closure used for
measuring tags_editor().copy_from. Instead compute a stable byte-slice once in
setup (for example let src_tags_bytes =
RawRecordView::new(&src_bytes).tags().as_bytes()) and then inside the
iter_batched_ref closure reconstruct a temporary RawRecordView (or otherwise
wrap that byte slice) and pass its tags() to rec.tags_editor().copy_from; this
keeps RawRecord::from(...) and the measured copy_from call in the hot path while
removing the one-time view construction from each iteration.
In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs`:
- Around line 230-261: Replace the manual index-based loops in the decode
helpers with simpler, safer idioms: in decode_u8_array use
data[..count].to_vec() (or data.get(..count).unwrap_or_default() variant if you
need bounds safety), and for decode_u16_array, decode_i16_array,
decode_i32_array and decode_f32_bits_array use
data.chunks_exact(n).take(count).map(|c|
<type>::from_le_bytes(c.try_into().unwrap())).collect() (where n is 2 for
u16/i16 and 4 for i32/f32 bits) to avoid manual index arithmetic and improve
readability; update each function (decode_u8_array, decode_u16_array,
decode_i16_array, decode_i32_array, decode_f32_bits_array) accordingly and
ensure you handle or assert chunk length if necessary.
- Around line 360-381: The test set_array_element_u8_writes_one_element
currently computes index as index % initial.len(), which biases lower indices;
instead generate the index based on the actual initial length by producing the
pair (initial, index) together — e.g. use proptest's prop_flat_map (or similar)
to first generate initial via proptest::collection::vec(any::<u8>(), 1..32) and
then, inside the flat_map closure, generate index from 0..initial.len(),
returning (initial, index) so the index distribution is uniform for the actual
array length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7216b721-0ffe-450b-90aa-16d5a83d4c11
📒 Files selected for processing (4)
Cargo.tomlbenches/raw_bam.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/tests/tags_editor_invariants.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
Adds comprehensive test and benchmark coverage for the raw-bam
view/editor API.
Consolidated benchmark target (`benches/raw_bam.rs`) — 32 criterion
benchmarks across 8 functional groups:
- view::* zero-copy header / CIGAR / seq / qual accessors
- tags::* iteration and batched string extraction
- editor::* length-changing tag mutations (int, float, string,
array, copy_from, normalize)
- mut::* fixed-length tag mutations
- length::* record length-changing setters (read name, CIGAR,
seq+qual)
- seq::* per-base and per-qual writes
- vs_recordbuf side-by-side raw-byte vs noodles RecordBuf
decode-mutate-reencode round-trip
Property tests (`crates/fgumi-raw-bam/tests/tags_editor_invariants.rs`):
- append+remove byte-level no-op for absent tags
- update_int/string/float round-trip across full value domains
- update_array_{u8,u16,i16,i32,f32} whole-payload round-trips
- set_array_element_{u8,u16,i16,i32,f32} preserves other elements
- set_bin round-trip across full u16 domain
- Mixed update/remove sequences preserve iterability
Edge-case unit tests (`crates/fgumi-raw-bam/src/raw_bam_record.rs`):
- Max-length (254-byte) read name succeeds; one over panics
- Zero-length sequence
- set_cigar_ops(&[]) clears CIGAR without touching aux
- Length-changing edits preserve B-array aux tags byte-for-byte
Part of #272.
40211c0 to
40ffceb
Compare
This branch was previously deployed
1 participant
Summary
Part 4 of 5 in the stacked series closing #272. Stacks on #280.
Adds benchmarks, property tests, and edge-case unit tests covering the new API. Pure test/bench additions — no source behavior change.
Benchmarks (
benches/)raw_bam_accessors.rs— hot-path accessor reads (flags, pos, read_name, cigar iter, quality scores, tag find)raw_bam_tags_editor.rs— tag editor in-place vs resize patternsraw_bam_length_changing.rs— length-changing edit baselinesraw_bam_vs_recordbuf.rs— side-by-side raw-byte vs noodlesRecordBufround-trip (measures actual speedup per op)Property tests
tests/tags_editor_invariants.rs:append(tag, v); remove(tag)is a byte-level no-op when tag absent (string + int)update_int/string/floatround-trip across full value domainsupdate_array_{u8,u16,i16,i32,f32}whole-payload round-tripsset_array_element_{u8,u16,i16,i32,f32}preserves other elementsset_binround-trip across fullu16domainaux_offsetstays valid)Edge-case unit tests (in
raw_bam_record.rs)set_cigar_ops(&[])clears CIGAR without touching auxTest plan
cargo ci-test— all new tests passcargo bench --bench raw_bam_vs_recordbuf— numbers captured in PR 5 descriptionStack
Part of #272. Supersedes #277.