feat(train): tiered (position-filtered) keep-set .sa build for Design-Z - #53
Merged
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughAdds keep-bed tiered suffix-array build support, query-time sparse-SA guards, a halo BED generator, and tests plus CI coverage for the new tiered paths. ChangesTiered Keep-Mask SA Build
Sequence Diagram(s)sequenceDiagram
participant CLI as Cmd::Build
participant Train as build_sidecar_from_pac_with_config
participant Meta as Meta::validate_with_file
participant Index as CollectContext
CLI->>Train: --keep-bed path.bed
Train->>Train: filter SA entries and build tiered training set
Train->>Meta: write tiered .sa metadata
Index->>Index: present_anchor and zigzag stall checks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prmi/src/sidecar/meta.rs (1)
370-398: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject
tiered=truewhensa.l_pacis missing.Validation currently skips all tiered-entry invariants if
l_pacis absent. For tiered sidecars, that permits metadata that later derives the wrong forward length fromsa_num.Proposed fix
- if let Some(l_pac) = self.sa.l_pac { + if self.sa.tiered == Some(true) && self.sa.l_pac.is_none() { + return Err(Error::SizeMismatch { + file: file.to_path_buf(), + detail: "tiered=true requires sa.l_pac to be set".to_string(), + }); + } + if let Some(l_pac) = self.sa.l_pac { let expected_entries = l_pac .checked_mul(2) .and_then(|v| v.checked_add(1))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prmi/src/sidecar/meta.rs` around lines 370 - 398, The validation logic for tiered sidecars currently skips all checks when l_pac is missing because all validation is gated within the if let Some(l_pac) block. Add an additional check after this block (in the else case or as a separate condition) to explicitly reject tiered sidecars when sa.l_pac is None by returning an Error::SizeMismatch with an appropriate detail message indicating that tiered sidecars require l_pac to be present. This ensures that metadata with tiered=true but missing l_pac is caught and prevents downstream issues with forward length derivation from sa_num.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi/examples/build_halo_bed.rs`:
- Around line 47-52: The FASTA parsing logic in the strip_prefix section
silently continues when encountering multiple FASTA headers, causing the first
contig name to be used for all sequences. Additionally, the BED contig
processing around lines 74-79 ignores the contig field from the BED file
entirely. To fix this, add a check in the FASTA header parsing block to reject
if contig is already set (preventing multiple headers), and add a validation in
the BED processing section to ensure the BED contig matches the FASTA contig
before processing. This will cause the program to fail with clear error messages
instead of silently producing misaligned coordinates.
- Around line 36-37: The flank variable is parsed without validation, allowing
negative values that cause invalid interval bounds in arithmetic operations. Add
an assertion after parsing flank (where `let flank: i64 =
a[5].parse().expect("flank");` is located) to validate that flank is within
acceptable bounds, similar to the pattern used for the k variable validation.
This assertion should reject negative or otherwise invalid flank values.
In `@prmi/src/cli.rs`:
- Around line 273-277: The validation guard that checks for the incompatibility
between keep_bed and with_isa currently uses anyhow::bail! for error handling,
but this is a user-input validation error that should use the crate's
Error::InvalidInput categorization instead. Replace the anyhow::bail! call in
the condition block (where keep_bed.is_some() && with_isa is checked) with a
return statement that constructs Error::InvalidInput with a detail field
containing the descriptive error message about the --keep-bed and --with-isa
incompatibility, ensuring consistency with other CLI input validation errors in
the crate.
In `@prmi/src/index/collect.rs`:
- Around line 275-283: The while loop condition at the start of the loop and the
slice operation in the mem_search call both perform unchecked arithmetic by
adding K to the start index, which can overflow or panic when slicing the
caller-provided read buffer. Replace the direct addition operations with
checked_add to safely compute start + K, and return false immediately if the
checked_add returns None (indicating overflow). This ensures fail-closed
behavior before any buffer slicing occurs, protecting against integer overflow
when working with caller-derived indices and buffer lengths.
In `@prmi/src/sidecar/meta.rs`:
- Around line 563-613: The tiered metadata tests
(tiered_sa_allows_fewer_entries_than_full, non_tiered_rejects_fewer_entries,
num_entries_above_max_rejected_even_when_tiered) only exercise mode "2" with
bytes_per_entry = 13, but this on-disk format change should be validated across
all memory modes. Create an additional test helper function similar to
tiered_toml that generates test TOML for mode "1" (with the appropriate
bytes_per_entry value for mode 1), then add corresponding test cases that verify
the tiered validation logic works correctly for mode "1" as well, ensuring the
same validation constraints are enforced consistently across all modes.
In `@prmi/src/train/mask.rs`:
- Around line 159-167: In the `keep_doubled_pos` function, the current logic
treats any position greater than 2*l_pac as a sentinel and returns true, which
is too permissive. Change the condition that handles coordinates beyond the
valid range to check for the exact sentinel position (pos == 2*l_pac) and return
true only for that specific case. For any other out-of-range positions (pos >
2*l_pac), add a fallback that returns false to reject malformed coordinates
instead of retaining them.
In `@prmi/src/train/mod.rs`:
- Around line 205-230: The issue is that while num_sa_entries correctly captures
the retained SA entry count when a keep-bed filter is applied, the .kmt writer
path still uses the full sa.len() instead of the retained count. Find where the
.kmt metadata is being written (likely in a writer or builder that takes sa_num
as a parameter) and replace the sa.len() call with num_sa_entries to ensure both
.sa/.meta and .kmt files use the same SA cardinality, which will keep k-mer
table bounds valid at read time.
In `@prmi/src/train/training_set.rs`:
- Around line 411-446: The keep_masked_training_set function currently ignores
mask and prior weighting configurations that are applied in masked_training_set,
such as mask_n_runs, homopolymer filtering, and mask_bed, causing inconsistent
behavior when using --keep-bed with mask options. To fix this, either add
MaskConfig, NBitmap, and Prior parameters to the keep_masked_training_set
function signature and apply the same filtering and weighting transformations
that masked_training_set applies to the keys and sa_indices before returning the
TrainingSet, or alternatively add validation at the CLI or build setup level to
explicitly reject incompatible combinations of --keep-bed with masking/prior
options.
In `@prmi/tests/z_keep_mask.rs`:
- Around line 273-291: The test currently only verifies self-consistency (occ_Z
<= occ_full) and that the call returns without hanging, but does not
independently verify that the stall guard path (search_pivot <=
entry_search_pivot) actually fires during execution. Add instrumentation to
track when the guard fires (through the smem_mns function or by adding a counter
in the search logic), then add an assertion after the loop in the test to verify
that the guard count is non-zero, providing independent oracle coverage for the
new search behavior as required by test conventions.
- Around line 390-399: The current assertion using assert_ne!(f, zz) is
insufficient because it only checks that the vectors differ overall, but zz
might still contain the full-span off-keep SMEM that should have been rejected.
Instead of comparing the full vectors, filter the SMEM entries from f to
identify those matching the full-span pattern (where m=0 and n=read_len-1), then
assert that none of these specific full-span SMEM entries are present in zz.
This ensures the keep-masked index properly rejected the off-keep full-span
match rather than just having additional or different entries.
---
Outside diff comments:
In `@prmi/src/sidecar/meta.rs`:
- Around line 370-398: The validation logic for tiered sidecars currently skips
all checks when l_pac is missing because all validation is gated within the if
let Some(l_pac) block. Add an additional check after this block (in the else
case or as a separate condition) to explicitly reject tiered sidecars when
sa.l_pac is None by returning an Error::SizeMismatch with an appropriate detail
message indicating that tiered sidecars require l_pac to be present. This
ensures that metadata with tiered=true but missing l_pac is caught and prevents
downstream issues with forward length derivation from sa_num.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f3a5958-b675-4228-a58c-82cf7e5e7ad1
📒 Files selected for processing (10)
prmi/examples/build_halo_bed.rsprmi/src/cli.rsprmi/src/index/collect.rsprmi/src/sidecar/meta.rsprmi/src/train/mask.rsprmi/src/train/mod.rsprmi/src/train/training_set.rsprmi/tests/mask.rsprmi/tests/sidecar_meta.rsprmi/tests/z_keep_mask.rs
Author
|
Addressed all 10 inline comments plus the outside-diff comment. Two notes on the less-mechanical ones:
|
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prmi/src/train/mod.rs (1)
248-292: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail fast when
--keep-bedleaves zero trainable 32-mers.An empty/comment-only or out-of-range keep BED can retain only the sentinel, producing an empty
TrainingSetthat is sent to the trainer. ReturnError::InvalidInputbefore training.Proposed fail-closed guard
} else { // Materialized path. The SA is in doubled coordinates over `text` // (length 2*l_pac+1), so the training set indexes bases/n_positions in // those same coordinates: the 0..=3 base array from `text` (1..=4, @@ let n_positions_2x = doubled_n_bitmap(&n_positions, &mask, text.len()); masked_training_set(&sa, &text_bases, &n_positions_2x, &mask, &config.prior) }; + if keep.is_some() && ts.is_empty() { + return Err(Error::InvalidInput { + detail: "--keep-bed retained no trainable 32-mers".into(), + }); + } let model = crate::train::trainer::train_with_config(&ts, l2_leaf_count, &config)?;Based on learnings, user-provided keep BED failures should use
Error::InvalidInputrather than surfacing as internal trainer failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prmi/src/train/mod.rs` around lines 248 - 292, When the keep option is provided and keep_masked_training_set() is called to generate the TrainingSet, add a validation check immediately after its return to ensure the TrainingSet is not empty. If the TrainingSet contains zero trainable 32-mers (which can happen when a keep BED is empty, contains only comments, or specifies out-of-range intervals), return Error::InvalidInput with a descriptive message before passing the training set to train_with_config(). This prevents empty training sets from reaching the trainer and surfacing as internal failures.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi/examples/build_halo_bed.rs`:
- Around line 36-38: The flank variable is parsed as i64 and only checked for
non-negative values, but large positive values can still cause overflow in
subsequent interval arithmetic operations. Parse flank as usize instead of i64
(which automatically prevents negative values), remove or update the assertion
that checks flank >= 0 since usize is unsigned, and update all downstream
interval arithmetic operations that use flank (in the range around lines
178-209) to use saturating arithmetic methods to prevent overflow.
- Around line 116-141: The k-mer counting logic currently uses only the forward
k-mer code as the key, but should use the canonical form to handle
reverse-complement matches. Replace all k-mer key references with the minimum of
the forward code and its reverse-complement: use min(code, revcomp(code, k))
instead of just code when calling count.entry(code), in_ex_kmer.insert(code,
true), and count.get(kmer) in the filter. Apply the same canonicalization
pattern to the second pass mentioned at lines 149-165 to ensure consistent halo
set membership across both the forward and reverse-complement representations.
- Around line 75-103: The BED file parsing logic in the lines iteration
(starting with for line in bf.lines()) needs to be more strict about input
validation to fail closed on malformed data. After reading each line, trim
whitespace from the input. Expand the skip conditions to include lines starting
with "browser" in addition to the existing checks for empty lines, comments, and
"track". After parsing the start and end values from c[1] and c[2], add
validation to ensure start < end and reject the row if this condition is not
met. Finally, instead of silently truncating intervals that extend past the
reference using e.min(n) in the bitmap fill loop, add a check to reject any
interval where end > n and emit an error before processing.
In `@prmi/src/index/collect.rs`:
- Around line 604-610: Replace the assignment `pivot = next_pivot` with a
checked arithmetic operation that advances from entry_search_pivot instead. Use
entry_search_pivot.checked_add(1) and bound the result to next_pivot to continue
the pass-1 scan from the next position rather than terminating early.
Additionally, add an oracle assertion after the stall guard fires to verify that
SMEM completeness is maintained and no rightward positions are skipped when
served reads are involved, such as by cross-validating against a full-SA
baseline.
In `@prmi/src/train/mod.rs`:
- Around line 495-496: The predicate used to determine whether to materialize
the n_positions_2x bitmap in the materialise_n assignment unnecessarily
allocates and scans a full bitmap by checking n_positions.is_some() even when no
Ns are present in the FASTA. Replace the n_positions.is_some() check with a
predicate that verifies n_positions actually contains data (such as checking if
the bitmap has a non-zero count or non-empty content) to avoid materialization
when the bitmap is empty and respect the documented empty-bitmap fast path.
In `@prmi/src/train/training_set.rs`:
- Around line 770-779: The test assertions in the training_set.rs file verify
that weights are either 3.5 or 1.0 and that some pairs have the BED weight of
3.5, but there is no assertion confirming that RC-half pairs actually receive
the 1.0 weight. This allows the test to pass even if all pairs are incorrectly
weighted as 3.5. Add a third assertion w.contains(&1.0) after the existing
assertions to verify that at least some RC-half pairs receive the expected 1.0
weight, tightening the test coverage.
---
Outside diff comments:
In `@prmi/src/train/mod.rs`:
- Around line 248-292: When the keep option is provided and
keep_masked_training_set() is called to generate the TrainingSet, add a
validation check immediately after its return to ensure the TrainingSet is not
empty. If the TrainingSet contains zero trainable 32-mers (which can happen when
a keep BED is empty, contains only comments, or specifies out-of-range
intervals), return Error::InvalidInput with a descriptive message before passing
the training set to train_with_config(). This prevents empty training sets from
reaching the trainer and surfacing as internal failures.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c1bbefc-78df-4aec-b768-9240a92563f5
📒 Files selected for processing (11)
.github/workflows/ci.ymlprmi/examples/build_halo_bed.rsprmi/src/cli.rsprmi/src/index/collect.rsprmi/src/sidecar/meta.rsprmi/src/train/mask.rsprmi/src/train/mod.rsprmi/src/train/training_set.rsprmi/tests/mask.rsprmi/tests/sidecar_meta.rsprmi/tests/z_keep_mask.rs
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prmi/src/train/mod.rs (1)
248-292: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject zero-pair tiered training before model fitting.
An empty or out-of-reference
--keep-bedcan retain only the sentinel, yieldingts.is_empty()and pushing a user-input failure intotrain_with_config. One-line fix: fail closed withError::InvalidInputbefore training.Proposed guard
masked_training_set(&sa, &text_bases, &n_positions_2x, &mask, &config.prior) }; + if ts.is_empty() { + return Err(Error::InvalidInput { + detail: "training set is empty; relax --keep-bed/--mask-* or use a reference with at least one 32-mer".into(), + }); + } let model = crate::train::trainer::train_with_config(&ts, l2_leaf_count, &config)?;Based on learnings, user-provided BED/config validation failures should use
Error::InvalidInput { detail: ... }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prmi/src/train/mod.rs` around lines 248 - 292, After the conditional block that creates the training set `ts` (the if/else chain involving keep_masked_training_set, streamed_training_set, and masked_training_set), add a validation guard that checks if the training set is empty using `ts.is_empty()`. If the training set is empty, return an `Error::InvalidInput` with a descriptive detail message before the call to `train_with_config`. This ensures user-input validation failures from empty or out-of-reference `--keep-bed` files are caught early and reported as input errors rather than propagating into the model fitting logic.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@prmi/src/train/mod.rs`:
- Around line 248-292: After the conditional block that creates the training set
`ts` (the if/else chain involving keep_masked_training_set,
streamed_training_set, and masked_training_set), add a validation guard that
checks if the training set is empty using `ts.is_empty()`. If the training set
is empty, return an `Error::InvalidInput` with a descriptive detail message
before the call to `train_with_config`. This ensures user-input validation
failures from empty or out-of-reference `--keep-bed` files are caught early and
reported as input errors rather than propagating into the model fitting logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce6db9af-74f7-440b-a3bc-88dd61c66f0d
📒 Files selected for processing (11)
.github/workflows/ci.ymlprmi/examples/build_halo_bed.rsprmi/src/cli.rsprmi/src/index/collect.rsprmi/src/sidecar/meta.rsprmi/src/train/mask.rsprmi/src/train/mod.rsprmi/src/train/training_set.rsprmi/tests/mask.rsprmi/tests/sidecar_meta.rsprmi/tests/z_keep_mask.rs
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi/examples/build_halo_bed.rs`:
- Around line 166-170: The halo membership counting needs to account for both
orientations in the `[Fwd||RC]` representation and properly check exome
intersection. First, when incrementing the count for canonical k-mers in the
saturation logic (at the `c.saturating_add(1)` call), check if the k-mer is
palindromic (where code equals revcomp_code(code, k)) and increment by 2 instead
of 1 to reflect both orientations. Second, replace the simple `if
in_exome[start]` check with logic that verifies the entire k-mer window
(spanning from start to start+k-1) intersects with exome regions, rather than
only checking if the start position is exonic, to correctly capture
boundary-spanning seeds.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3edb0c34-5eaa-409b-a8ef-1932dee18f29
📒 Files selected for processing (11)
.github/workflows/ci.ymlprmi/examples/build_halo_bed.rsprmi/src/cli.rsprmi/src/index/collect.rsprmi/src/sidecar/meta.rsprmi/src/train/mask.rsprmi/src/train/mod.rsprmi/src/train/training_set.rsprmi/tests/mask.rsprmi/tests/sidecar_meta.rsprmi/tests/z_keep_mask.rs
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi/src/index/collect.rs`:
- Line 333: In the mem_search call on line 333, remove the unsafe narrowing cast
from match_len (u64) to usize before comparing with K. Instead, compare
match_len directly as u64 by widening the constant K to u64 in the comparison,
ensuring the full 64-bit value is preserved on all platforms including 32-bit
systems.
In `@prmi/src/train/mask.rs`:
- Around line 42-44: The `keep_bed_path` field is documented as being for
`.meta` provenance and is preserved by `mask_config_from_cli`, but it is not
being written to the metadata that gets stored in the sidecar (which currently
only records `tiered` and `masked_bed`). Either add an optional `keep_bed_path`
field to the metadata structure and ensure it is populated from
`mask.keep_bed_path` when recording metadata, or remove the `keep_bed_path`
field from the struct and update its documentation if provenance tracking is not
actually needed. Ensure the implementation aligns with the documented intent.
In `@prmi/src/train/mod.rs`:
- Around line 205-218: Move the validation of the keep-bed to immediately after
genome_len is determined, before the build_gsa function is called. Add a check
to detect when the keep-bed has no intervals or when all intervals start at or
after genome_len (which would only retain the sentinel and cause failure later).
Return an Error::InvalidInput with appropriate detail message for these invalid
keep-bed cases, preventing unnecessary construction of the full doubled GSA when
the build is guaranteed to fail.
In `@prmi/tests/z_keep_mask.rs`:
- Around line 120-135: The current test for present_anchor only covers cases
where reads have no N's, which means implementations that incorrectly check only
read[0..32] or scan later kept 32-mers could still pass. Add two additional test
cases after the existing assertions: first, create a read where the first window
contains N characters but later 32-mers are N-free (and the first N-free 32-mer
is in a kept region), and verify that both z and full correctly identify it as
present; second, create a read where the first N-free 32-mer is in an off-keep
region even though a later 32-mer falls in the kept region, and verify that z
correctly rejects it while full accepts it. These additions ensure
present_anchor properly validates against its first-N-free contract.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc53ce5a-7a00-4a37-b690-d37b3e9a1b2c
📒 Files selected for processing (11)
.github/workflows/ci.ymlprmi/examples/build_halo_bed.rsprmi/src/cli.rsprmi/src/index/collect.rsprmi/src/sidecar/meta.rsprmi/src/train/mask.rsprmi/src/train/mod.rsprmi/src/train/training_set.rsprmi/tests/mask.rsprmi/tests/sidecar_meta.rsprmi/tests/z_keep_mask.rs
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi/examples/build_halo_bed.rs`:
- Around line 78-86: The FASTA parsing loop in build_halo_bed is pushing raw
bytes from line.bytes(), so CRLF inputs leave a trailing carriage return that
becomes a phantom sentinel base and shifts coordinates. Trim each sequence line
before iterating its bytes in the sequence-reading path, matching the existing
BED line handling, so only real nucleotide characters are converted and
appended.
In `@prmi/tests/sidecar_meta.rs`:
- Around line 64-79: The new tiered keep_bed round-trip test only exercises the
sample() mode 1 metadata, so add a second round-trip case that uses the valid
mode 2 field combination (bytes_per_entry = 13, encoding =
"packed_lo8_hi32_key64", stored_keys = Some(true)) to cover the same
keep_bed/tiered provenance path. Update the sidecar metadata tests in
roundtrip_tiered_keep_bed_provenance, using the existing Meta and sample
helpers, so the new on-disk fields are verified across both memory modes.
In `@prmi/tests/z_keep_mask.rs`:
- Around line 303-315: The occ-subset oracle in z_keep_mask.rs is too weak
because it only asserts when smem_mns(&full, ...) already emitted the same
(m,n), which lets Z-only spans bypass the no-inflation contract. Update the test
around smem_mns and z_set so every Z span computes its full-index occurrence
oracle directly from full using the existing search helpers (for example
full.mem_search on the read slice for that span) and compare s_z against that
value instead of skipping missing entries. Keep the assertion tied to the same
(m, n) span variables so the coverage remains independent and exact.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f8d82d7-0f9d-48b2-9a91-6e379cd0e79f
📒 Files selected for processing (11)
.github/workflows/ci.ymlprmi/examples/build_halo_bed.rsprmi/src/cli.rsprmi/src/index/collect.rsprmi/src/sidecar/meta.rsprmi/src/train/mask.rsprmi/src/train/mod.rsprmi/src/train/training_set.rsprmi/tests/mask.rsprmi/tests/sidecar_meta.rsprmi/tests/z_keep_mask.rs
Build the .sa + RMI model over only keep-set suffix positions while retaining the full genome text/.pac, so served reads' SMEMs are byte-identical to the whole-genome index and positions stay native genome coordinates (no translation table). The full build is unchanged when no keep-set is given. - MaskConfig::keep_bed + RC-symmetric keep_doubled_pos (keeps a forward position and its reverse-complement image together, as the RC-span/zigzag logic needs). - SA-write keep-filter in build_sidecar_core; keep_masked_training_set trains the RMI on compacted ranks 0..N_kept matching the filtered .sa write order; meta.sa.l_pac stays the full genome length; .isa skipped under a keep-mask. - meta.sa.tiered flag authorizes num_entries < 2*l_pac+1 (a full SA stays exact). - --keep-bed CLI flag (incompatible with --with-isa).
The reseed/zigzag walks (zz_step1, zz_step1_reseed) assumed the full-genome SA, where the walk always advances. A position-filtered (tiered) SA can stall them: the left RC span and the right extension disagree over partially-present copies, leaving search_pivot stationary while right > 0, so the existing right==0 stall guard never fires and the walk loops forever. Break when search_pivot does not advance past its value at the top of the iteration. This is a strict no-op on the full SA (the walk always advances there), so full-genome byte-identity is preserved (the oracle/lockstep proptests stay green); it only terminates the tiered-SA stall. Also adds present_anchor: a one-probe tiered-dispatch pre-reject (the read's first 32-mer present in this index?), far cheaper than a full collect_smems whose failed reseeds on an absent read cost hundreds of probes.
…ject tests Synthetic proofs for Design Z (no committed fixtures; sequence built in-test): served reads are byte-identical (m,n,s) and off-keep reads diverge; the compacted-rank model is well-fit; a repetitive low-copy case exercises the forward-progress guard's firing path (it hangs without the guard); served SMEM genome positions match the full index end-to-end; and present_anchor discriminates served from off-keep reads.
…et builder Measurement tool: emits the Design Z keep-set BED (exome targets plus the genome-count-capped low-copy homology halo, each flanked) for a single-contig reference. Two passes over the reference count k-mers and recover the positions of in-exome low-copy k-mers. Not shipped (in-RAM; chr22-scale).
…or docstring Address review: build_halo_bed wrote a hardcoded 'chr22' contig in its output BED regardless of the input reference; take the contig name from the first FASTA header instead (the tool is single-contig). Clarify present_anchor's docstring — it inspects only the FIRST N-free 32-mer window, not 'some' window.
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 25, 2026
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
1 participant
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.
Design-Z stack 2/6 — base:
main.Tiered (position-filtered) keep-set
.sabuild mode: when--keep-bedis given, the on-disk suffix array retains only entries whose forward reference coordinate lies in the keep-set, applied RC-symmetrically over the doubled text (keep_doubled_pos). The full genome text/.pacis unchanged, somatch_lenand SA positions stay native; only the entry count shrinks. Includes forward-progress guards for the tiered zigzag walks, byte-identity/guard-firing tests vs the full index, and a single-contigbuild_halo_bedkeep-set example.Dual-reviewed (coderabbit --agent + local CR); fixes applied (contig name in build_halo_bed; present_anchor docstring).
cargo build/clippy/testgreen.Summary by CodeRabbit
prmi build --keep-bedfor RC-symmetric, position-filtered tiered indexes that shrink the on-disk suffix array while preserving genome coordinate interpretation (incompatible with--with-isa)..sasidecar metadata with optional tiering/keep-BED fields and updated tiered validation.spectrum-probe-countfeature-gated test run.