Conversation
…and improve documentation
…ameters and validation
…nd upload to release
…ring in alignment
- Removed dependency on `fftconvolve` crate by implementing a custom FFT cross-correlation function `fft_cross_correlate_full`. - Updated `star_align_tics_fft` and `progressive_align_tics_fft` functions to utilize the new FFT implementation. - Cleaned up imports and removed unused dependencies in `Cargo.toml` and relevant source files. - Added unit tests for the new FFT cross-correlation function to ensure correctness.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds an arycal-debug-alignment CLI that emits per-precursor SVG/TSV/JSON debug artifacts; centralizes config loading; extends Runner with batch helpers; replaces FFT cross-correlation with rustfft-based convolution; refactors DTW RT-mapping to use path indices; adds peak-mapping candidate inspection; and updates CI/workspace to build and publish both binaries. ChangesDebug Alignment Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/arycal-cli/src/lib.rs (1)
1084-1089:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve precursors that have no feature rows.
If
fetch_feature_data_for_precursor_batchomits a precursor ID, this branch drops that precursor fromresultsentirely. The older per-precursor flow still returned an emptyPrecursorAlignmentResultin that case, so this changes behavior and can silently suppress outputs/debug artifacts for otherwise valid precursors.Suggested fix
- let feature_data = match all_feature_data.get(precursor_id) { - Some(f) => f, - None => { - log::trace!("Feature data not found for precursor {}", precursor_id); - return None; - } - }; + let feature_data = match all_feature_data.get(precursor_id) { + Some(f) => f.as_slice(), + None => { + log::trace!( + "Feature data not found for precursor {}, continuing with an empty feature set", + precursor_id + ); + &[] + } + };🤖 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 `@crates/arycal-cli/src/lib.rs` around lines 1084 - 1089, The match on all_feature_data.get(precursor_id) returns None and the code returns None, which drops that precursor from results; instead, when fetch_feature_data_for_precursor_batch omitted a precursor, treat it as having no feature rows and produce an empty PrecursorAlignmentResult rather than returning None. Update the branch that handles None for all_feature_data.get(precursor_id) to create/use an empty feature-data placeholder (e.g., empty Vec/Default) and continue building/inserting a PrecursorAlignmentResult (or use PrecursorAlignmentResult::default()) for precursor_id so precursors with no rows are preserved in results; keep references to precursor_id, all_feature_data, fetch_feature_data_for_precursor_batch, and PrecursorAlignmentResult to locate the change.
🧹 Nitpick comments (1)
crates/arycal-cli/src/lib.rs (1)
297-300: 🏗️ Heavy liftThis regrouping clones every
TransitionGroupon the hot path.
all_precursor_groupsalready owns the loaded chromatograms, butgroups.get(...).cloned()copies eachTransitionGroupinto a second structure, andprepare_xics_batchclones that vector again on Line 900-903. For large batches that can materially inflate memory use and undo the intended batch-processing win. Consider restructuring this to move/share the groups once instead of cloning them twice.🤖 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 `@crates/arycal-cli/src/lib.rs` around lines 297 - 300, The current filter_map uses groups.get(...).cloned(), which copies each TransitionGroup and later prepare_xics_batch clones the vector again, doubling memory; instead, change the pipeline to transfer or borrow the groups once: either (A) take ownership from the source map by using remove/into_iter to move the Vec<TransitionGroup> for precursor.precursor_id into the collection so no clone occurs, or (B) collect references (Vec<&TransitionGroup>) and update prepare_xics_batch to accept references/slices (e.g. &[&TransitionGroup]) so no data is cloned; update the call sites and the prepare_xics_batch signature accordingly to accept moved ownership or references to avoid the extra clones of TransitionGroup.
🤖 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 `@crates/arycal-cli/src/bin/arycal-debug-alignment.rs`:
- Around line 233-236: The code currently calls
chrom.chromatogram.metadata.get("basename").unwrap() which panics if the
metadata key is missing; change this to use the basename value or fall back to
the chromatogram native id (e.g., chrom.chromatogram.native_id) without
panicking—e.g., replace the unwrap with a safe lookup that maps to a clone of
the found string or uses native_id.to_string() when None so the tuple
(basename_or_native, chrom) never panics on missing metadata.
In `@crates/arycal-cloudpath/src/openms_xic_parquet.rs`:
- Around line 169-177: The current collection into precursor_to_transitions
using precursors.iter().map(...).collect() will overwrite entries for duplicate
p.precursor_id and drop earlier transition_ids; change the logic to iterate over
precursors and use the HashMap::entry API (or fold) to get_or_insert_with a
HashSet for each p.precursor_id and extend or insert all p.transition_ids
(converting to i64) into that set so transitions from duplicate precursor rows
are merged rather than replaced; locate the code that defines
precursor_to_transitions and the mapping using p.precursor_id and
p.transition_ids to implement this fix.
In `@crates/arycal/src/alignment/fast_fourier_lag_dtw.rs`:
- Around line 65-71: The DTW path indices are 1-based, so when building slices
from the path (in the mapping that uses aligned_chrom.retention_times and
aligned_chrom.intensities) you must subtract 1 before indexing; update the
closure that currently maps |&(_, j)| (aligned_chrom.retention_times[j],
aligned_chrom.intensities[j]) to use j - 1 (and ensure the path was filtered for
j > 0 like in create_fft_dtw_rt_mapping) so you access
(aligned_chrom.retention_times[j - 1], aligned_chrom.intensities[j - 1]) and
avoid skipping the first data point.
In `@crates/arycal/src/alignment/fast_fourier_lag.rs`:
- Around line 222-225: The code calls find_lag_with_max_correlation on the
result of fft_cross_correlate_full without checking for an empty Vec, which can
panic; update the places where you compute let cross_corr =
fft_cross_correlate_full(...) (and then let lag =
find_lag_with_max_correlation(&cross_corr)) to first check if
cross_corr.is_empty() and handle that case (e.g., set lag to 0, skip alignment,
or return an Err) instead of calling find_lag_with_max_correlation; make the
same guard in all occurrences (the blocks using fft_cross_correlate_full and
find_lag_with_max_correlation) so empty intensity inputs are safely handled.
- Around line 33-38: The FFT cross-correlation currently documents computing
conv(reference, reverse(query)) but downstream code
(find_lag_with_max_correlation) assumes a center-zero convention (len/2), which
is only correct for equal-length inputs; for unequal lengths the true zero-lag
index is query.len() - 1. Update the code/path of least surprise by making the
lag convention explicit and consistent: either (A) keep
fft_cross_correlate_full(reference, query) as conv(reference, reverse(query))
and change find_lag_with_max_correlation to treat zero-lag at index query.len()
- 1 when converting index→lag, or (B) shift the vector returned by
fft_cross_correlate_full so its zero-lag is at reference.len()/2 (document the
change). Prefer option A: modify find_lag_with_max_correlation to compute
zero_lag_index = query.len().saturating_sub(1) and use that when converting peak
index to lag; update comments in fft_cross_correlate_full and
find_lag_with_max_correlation to state the chosen convention.
---
Outside diff comments:
In `@crates/arycal-cli/src/lib.rs`:
- Around line 1084-1089: The match on all_feature_data.get(precursor_id) returns
None and the code returns None, which drops that precursor from results;
instead, when fetch_feature_data_for_precursor_batch omitted a precursor, treat
it as having no feature rows and produce an empty PrecursorAlignmentResult
rather than returning None. Update the branch that handles None for
all_feature_data.get(precursor_id) to create/use an empty feature-data
placeholder (e.g., empty Vec/Default) and continue building/inserting a
PrecursorAlignmentResult (or use PrecursorAlignmentResult::default()) for
precursor_id so precursors with no rows are preserved in results; keep
references to precursor_id, all_feature_data,
fetch_feature_data_for_precursor_batch, and PrecursorAlignmentResult to locate
the change.
---
Nitpick comments:
In `@crates/arycal-cli/src/lib.rs`:
- Around line 297-300: The current filter_map uses groups.get(...).cloned(),
which copies each TransitionGroup and later prepare_xics_batch clones the vector
again, doubling memory; instead, change the pipeline to transfer or borrow the
groups once: either (A) take ownership from the source map by using
remove/into_iter to move the Vec<TransitionGroup> for precursor.precursor_id
into the collection so no clone occurs, or (B) collect references
(Vec<&TransitionGroup>) and update prepare_xics_batch to accept
references/slices (e.g. &[&TransitionGroup]) so no data is cloned; update the
call sites and the prepare_xics_batch signature accordingly to accept moved
ownership or references to avoid the extra clones of TransitionGroup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3df7bc58-f95a-4994-a713-c00953f919d4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/rust-release.ymlCargo.tomlcrates/arycal-cli/src/bin/arycal-debug-alignment.rscrates/arycal-cli/src/input.rscrates/arycal-cli/src/lib.rscrates/arycal-cloudpath/src/openms_xic_parquet.rscrates/arycal/Cargo.tomlcrates/arycal/src/alignment/alignment.rscrates/arycal/src/alignment/fast_fourier_lag.rscrates/arycal/src/alignment/fast_fourier_lag_dtw.rs
💤 Files with no reviewable changes (1)
- crates/arycal/Cargo.toml
… generation methods - Introduced `stratified_shuffle_peak_tuples` function to shuffle peak tuples based on retention time and width. - Added `create_decoy_peaks_by_stratified_shuffling` to generate decoy peaks using stratified shuffling. - Implemented `create_decoy_peaks_by_candidate_hard_negative` to select hard negatives from real candidate features, preferring nearest wrong features within a specified RT tolerance. - Refactored existing decoy peak creation methods for better clarity and performance. - Added unit tests for new functionality to ensure correctness of peak shuffling and candidate selection logic.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/arycal-cli/src/main.rs (1)
186-191:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHelp text contradicts the documented default.
The CLI help says
default = # of CPUs/2, but the embedded template (line 76) and the actual default (saturating_sub(1)) use# of CPUs - 1. Align the help string to avoid user confusion.📝 Proposed fix
- .help("Number of threads for parallel computing (default = # of CPUs/2)") + .help("Number of threads for parallel computing (default = # of CPUs - 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 `@crates/arycal-cli/src/main.rs` around lines 186 - 191, The help string for the "threads" argument is incorrect—update the help text in Arg::new("threads") to reflect the actual default used elsewhere (saturating_sub(1)), e.g. change "default = # of CPUs/2" to "default = # of CPUs - 1" (or another phrasing that explicitly matches the implementation and the embedded template) so the help output aligns with the actual default behavior.crates/arycal-cli/src/lib.rs (1)
1418-1423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlignment scores silently disabled in batch path — bridge the divergence or remove dead code.
process_peak_mappings(line 1423) always returnsalignment_scores = HashMap::new()with the previous computation call commented out. The writer callself.write_aligned_score_results_to_db(...)is commented out in the non-MPIrun()path (line 483) and absent from the MPI branch. Meanwhile, the legacyprocess_precursorpath (line 813) still computes scores. Result: batch-mode users never getFullTraceAlignmentScores, the struct field is dead weight, and behavior diverges between code paths.Either restore the computation and writer call in the batch path, or remove the dead
alignment_scoresfield andwrite_aligned_score_results_to_dbmethod entirely.🤖 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 `@crates/arycal-cli/src/lib.rs` around lines 1418 - 1423, process_peak_mappings currently sets alignment_scores = HashMap::new() (and the compute_alignment_scores call is commented out) which causes batch/MPI runs to never produce FullTraceAlignmentScores while the legacy process_precursor path still computes them; either restore score computation and persistence or remove the dead field/method. To fix, choose one: (A) restore compute_alignment_scores(aligned.aligned_chromatograms.clone()) inside process_peak_mappings and ensure the batch and MPI branches in run() call write_aligned_score_results_to_db(...) (un-comment or add the call) so alignment_scores flows to the DB writer, or (B) remove the alignment_scores field from structs, delete write_aligned_score_results_to_db and all related references (including any commented writer calls and tests) to eliminate dead code; update process_precursor accordingly to match the chosen behavior. Ensure references: process_peak_mappings, compute_alignment_scores, write_aligned_score_results_to_db, run, process_precursor, and FullTraceAlignmentScores are consistently updated.
♻️ Duplicate comments (1)
crates/arycal-common/src/config.rs (1)
527-531:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame
available_parallelism().unwrap()panic risk as inmain.rs.Mirror the fix proposed for
crates/arycal-cli/src/main.rslines 24-28 here so a failedavailable_parallelism()call doesn't panic when constructing a defaultOpenSwathConfig.🛡️ Proposed fix
- threads: std::thread::available_parallelism() - .unwrap() - .get() - .saturating_sub(2) - .max(1), + threads: std::thread::available_parallelism() + .map(|n| n.get()) + .unwrap_or(1) + .saturating_sub(2) + .max(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 `@crates/arycal-common/src/config.rs` around lines 527 - 531, The threads field initialization in OpenSwathConfig uses std::thread::available_parallelism().unwrap() which can panic; mirror the fix from crates/arycal-cli by handling the Result safely: call std::thread::available_parallelism(), map to .get() on success, fallback to a safe default (e.g., 1) on Err, then apply saturating_sub(2) and .max(1) as before. Update the threads initializer in the OpenSwathConfig/default construction to use this non-panicking pattern so construction never panics on unavailable parallelism.
🧹 Nitpick comments (3)
crates/arycal/src/scoring.rs (2)
945-952: ⚡ Quick winFall-through warning fires per precursor; validate once at config load.
If a user specifies an unknown
decoy_peak_mapping_method, thislog::warn!is emitted every timeprocess_precursorruns — and the same arm is duplicated inprocess_peak_mappings(lib.rs lines 1474-1481). For large precursor batches that's a lot of identical warnings. Validate the value once when the config is loaded (e.g. inInput::from_config_path) and either error out or canonicalize to"shuffle_stratified"so the per-precursor path can stay quiet.🤖 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 `@crates/arycal/src/scoring.rs` around lines 945 - 952, The warning about unknown decoy_peak_mapping_method is being emitted per precursor in process_precursor and duplicated in process_peak_mappings; instead validate and canonicalize this setting once during configuration parsing (e.g. inside Input::from_config_path) by checking the decoy_peak_mapping_method value and either return an error or normalize unknown values to "shuffle_stratified" (or a chosen default); remove or silence the per-precursor fall-through log paths (references: decoy_peak_mapping_method, process_precursor, process_peak_mappings, Input::from_config_path) so runtime loops no longer emit repeated identical warnings.
135-147: 💤 Low valueSattolo correctly used for derangement; consider documenting the cyclic-only bias.
sample_derangementusesrng.random_range(0..i)(exclusive ofi), i.e. Sattolo's algorithm, so the resulting permutation has no fixed points — a derangement. Note that this is biased: Sattolo only ever produces cyclic permutations, which is a strict subset of all derangements (e.g., forn=4the disjoint-pair derangements(2,3,0,1)etc. cannot be sampled). For decoy generation that bias is fine; flagging only because the function name implies general derangement sampling.📝 Optional doc clarification
-fn sample_derangement<R: Rng + ?Sized>(len: usize, rng: &mut R) -> Option<Vec<usize>> { +/// Returns a uniformly random *cyclic* permutation of `0..len` using Sattolo's +/// algorithm. The result has no fixed points (i.e. is a derangement) but is +/// restricted to the cyclic subset of all derangements. +fn sample_derangement<R: Rng + ?Sized>(len: usize, rng: &mut R) -> Option<Vec<usize>> {🤖 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 `@crates/arycal/src/scoring.rs` around lines 135 - 147, The function sample_derangement implements Sattolo's algorithm (using rng.random_range(0..i) in the reverse loop) which only generates cyclic derangements, not all derangements; update this by either renaming sample_derangement to sample_sattolo_derangement to signal the cyclic-only bias or change the implementation to an unbiased derangement sampler, and add a doc comment on sample_derangement (or the new name) that explicitly states the Sattolo/cyclic-only behavior and its implications for decoy generation.crates/arycal-cli/src/lib.rs (1)
1247-1283: ⚡ Quick winRedundant reconstruction of
precursor_run_setspurely for logging.
fetch_feature_data_for_aligned_batchalready builds the sameVec<(i32, Vec<String>)>internally (lines 345-362). Rebuilding it here only to compute counts for the debug log doubles the work and clones everybasename. Either expose the run counts from the helper, or compute them directly fromaligned_batchwithout the intermediate vector.♻️ Proposed fix — derive counts straight from `aligned_batch`
- let start_time = Instant::now(); - let all_feature_data = self.fetch_feature_data_for_aligned_batch(&aligned_batch)?; - let precursor_run_sets: Vec<(i32, Vec<String>)> = aligned_batch - .iter() - .map(|(precursor_id, aligned)| { - let runs = aligned - .aligned_chromatograms - .iter() - .map(|chrom| { - chrom - .chromatogram - .metadata - .get("basename") - .unwrap() - .to_string() - }) - .collect(); - (*precursor_id, runs) - }) - .collect(); - log::debug!( - "Fetching feature data for {:?} precursors for {:?} runs took: {:?} ({:?} MiB)", - precursor_run_sets.len(), - precursor_run_sets - .iter() - .map(|(_, runs)| runs.len()) - .sum::<usize>(), - start_time.elapsed(), - all_feature_data.deep_size_of() / 1024 / 1024 - ); + let start_time = Instant::now(); + let all_feature_data = self.fetch_feature_data_for_aligned_batch(&aligned_batch)?; + let total_runs: usize = aligned_batch + .values() + .map(|aligned| aligned.aligned_chromatograms.len()) + .sum(); + log::debug!( + "Fetched feature data for {} precursors across {} runs in {:?} ({} MiB)", + aligned_batch.len(), + total_runs, + start_time.elapsed(), + all_feature_data.deep_size_of() / 1024 / 1024 + );🤖 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 `@crates/arycal-cli/src/lib.rs` around lines 1247 - 1283, The debug block currently rebuilds `precursor_run_sets` by cloning every `basename`, which duplicates work done in `fetch_feature_data_for_aligned_batch`; remove that reconstruction and instead compute the counts directly from `aligned_batch` (e.g., use aligned_batch.len() for precursor count and sum over each aligned.aligned_chromatograms.len() for run count) or modify `fetch_feature_data_for_aligned_batch` to return run counts alongside `all_feature_data`; update the `log::debug!` call in the function containing `let start_time = Instant::now();` to use those computed counts (referencing `aligned_batch` and `fetch_feature_data_for_aligned_batch`) so no extra cloning occurs.
🤖 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 `@crates/arycal-cli/src/main.rs`:
- Around line 24-28: The current uses of
std::thread::available_parallelism().unwrap() are unsafe in restricted/sandboxed
environments; replace each unwrap() with a graceful fallback using .map(|n|
n.get()).unwrap_or(1). Specifically: in the threads field initialization (the
struct field named threads) change the call chain to .map(|n|
n.get()).unwrap_or(1).saturating_sub(1).max(1) (i.e., get the usize or default
to 1 before subtracting); in Input::default() (the Input::default
implementation) apply the same .map(|n| n.get()).unwrap_or(1) pattern; and in
the config default where .saturating_sub(2) is used (the Config/default or the
config-related initialization that currently does .saturating_sub(2)), use
.map(|n| n.get()).unwrap_or(1).saturating_sub(2).max(1) so the fallback is
applied before subtraction.
- Around line 9-15: Remove the duplicate conditional import of rlimit: delete
the first #[cfg(not(target_os = "windows"))] use rlimit::{setrlimit, Resource};
line so only the later #[cfg(not(target_os = "windows"))] use
rlimit::{getrlimit, setrlimit, Resource}; remains; this ensures setrlimit and
Resource are imported exactly once and getrlimit is available where needed
(refer to the use of setrlimit, getrlimit, and Resource in main.rs).
---
Outside diff comments:
In `@crates/arycal-cli/src/lib.rs`:
- Around line 1418-1423: process_peak_mappings currently sets alignment_scores =
HashMap::new() (and the compute_alignment_scores call is commented out) which
causes batch/MPI runs to never produce FullTraceAlignmentScores while the legacy
process_precursor path still computes them; either restore score computation and
persistence or remove the dead field/method. To fix, choose one: (A) restore
compute_alignment_scores(aligned.aligned_chromatograms.clone()) inside
process_peak_mappings and ensure the batch and MPI branches in run() call
write_aligned_score_results_to_db(...) (un-comment or add the call) so
alignment_scores flows to the DB writer, or (B) remove the alignment_scores
field from structs, delete write_aligned_score_results_to_db and all related
references (including any commented writer calls and tests) to eliminate dead
code; update process_precursor accordingly to match the chosen behavior. Ensure
references: process_peak_mappings, compute_alignment_scores,
write_aligned_score_results_to_db, run, process_precursor, and
FullTraceAlignmentScores are consistently updated.
In `@crates/arycal-cli/src/main.rs`:
- Around line 186-191: The help string for the "threads" argument is
incorrect—update the help text in Arg::new("threads") to reflect the actual
default used elsewhere (saturating_sub(1)), e.g. change "default = # of CPUs/2"
to "default = # of CPUs - 1" (or another phrasing that explicitly matches the
implementation and the embedded template) so the help output aligns with the
actual default behavior.
---
Duplicate comments:
In `@crates/arycal-common/src/config.rs`:
- Around line 527-531: The threads field initialization in OpenSwathConfig uses
std::thread::available_parallelism().unwrap() which can panic; mirror the fix
from crates/arycal-cli by handling the Result safely: call
std::thread::available_parallelism(), map to .get() on success, fallback to a
safe default (e.g., 1) on Err, then apply saturating_sub(2) and .max(1) as
before. Update the threads initializer in the OpenSwathConfig/default
construction to use this non-panicking pattern so construction never panics on
unavailable parallelism.
---
Nitpick comments:
In `@crates/arycal-cli/src/lib.rs`:
- Around line 1247-1283: The debug block currently rebuilds `precursor_run_sets`
by cloning every `basename`, which duplicates work done in
`fetch_feature_data_for_aligned_batch`; remove that reconstruction and instead
compute the counts directly from `aligned_batch` (e.g., use aligned_batch.len()
for precursor count and sum over each aligned.aligned_chromatograms.len() for
run count) or modify `fetch_feature_data_for_aligned_batch` to return run counts
alongside `all_feature_data`; update the `log::debug!` call in the function
containing `let start_time = Instant::now();` to use those computed counts
(referencing `aligned_batch` and `fetch_feature_data_for_aligned_batch`) so no
extra cloning occurs.
In `@crates/arycal/src/scoring.rs`:
- Around line 945-952: The warning about unknown decoy_peak_mapping_method is
being emitted per precursor in process_precursor and duplicated in
process_peak_mappings; instead validate and canonicalize this setting once
during configuration parsing (e.g. inside Input::from_config_path) by checking
the decoy_peak_mapping_method value and either return an error or normalize
unknown values to "shuffle_stratified" (or a chosen default); remove or silence
the per-precursor fall-through log paths (references: decoy_peak_mapping_method,
process_precursor, process_peak_mappings, Input::from_config_path) so runtime
loops no longer emit repeated identical warnings.
- Around line 135-147: The function sample_derangement implements Sattolo's
algorithm (using rng.random_range(0..i) in the reverse loop) which only
generates cyclic derangements, not all derangements; update this by either
renaming sample_derangement to sample_sattolo_derangement to signal the
cyclic-only bias or change the implementation to an unbiased derangement
sampler, and add a doc comment on sample_derangement (or the new name) that
explicitly states the Sattolo/cyclic-only behavior and its implications for
decoy generation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b332ec68-b05f-4187-8578-6b4d2e005e69
📒 Files selected for processing (6)
crates/arycal-cli/src/input.rscrates/arycal-cli/src/lib.rscrates/arycal-cli/src/main.rscrates/arycal-cli/src/output.rscrates/arycal-common/src/config.rscrates/arycal/src/scoring.rs
✅ Files skipped from review due to trivial changes (1)
- crates/arycal-cli/src/output.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/arycal-cli/src/input.rs
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.
This pull request introduces several improvements and refactorings to the build pipeline, CLI input handling, and internal data processing for the
arycalproject. The most significant changes are the addition of a new binary (arycal-debug-alignment) to the build and release process, enhancements to batch data processing in the CLI, and stricter filtering of chromatogram data in the OpenMS XIC Parquet reader. Additionally, new debug data structures for alignment inspection have been added.Build and Release Pipeline:
arycal-debug-alignmentas a second binary to all build targets and updated the GitHub Actions workflow to build, artifact, and upload botharycalandarycal-debug-alignmentfor all supported platforms. This ensures both binaries are available in releases and artifacts. [1] [2] [3] [4] [5] [6]CLI Input Handling and Refactoring:
Inputstruct's loading logic to centralize parameter inference and validation in a newfrom_config_pathmethod, ensuring consistent and correct initialization from configuration files. [1] [2]Batch Data Processing Improvements:
Runnerstruct (get_precursors_by_ids,read_transition_groups_batch,fetch_feature_data_for_aligned_batch, andprocess_peak_mappings_batch_with_feature_data) to improve performance and code clarity when handling large precursor and feature datasets. Refactored existing methods to utilize these batch operations. [1] [2] [3] [4]Chromatogram Data Filtering:
Alignment Debugging Structures:
PeakMappingCandidateDebug,PeakMappingInspection, andFlattenedFeatureCandidate) to the alignment module to facilitate inspection and debugging of peak mapping and alignment logic. [1] [2]These changes collectively improve the robustness, maintainability, and debuggability of the codebase.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Tests