chores and performance boost#9
chores and performance boost#9AdilZouitine merged 2 commits intoonline-ml:masterfrom OctopusTakopi:master
Conversation
* update crates * fix typos * make clippy happy * update info link perf: * Static Dispatch: By making Rolling<'a, U, F> generic over U: RollableUnivariate<F>, method calls (update, revert, get) use static dispatch (monomorphized code) instead of dynamic (vtable) * same Static Dispatch techinic for iter.
There was a problem hiding this comment.
Pull Request Overview
This PR performs general maintenance (dependency bumps, typo fixes, Clippy cleanups) and applies a performance optimization by replacing dynamic dispatch with static dispatch in two core abstractions.
- Updated
Rollingto be generic over the concreteRollableUnivariatetype, preallocating its window buffer. - Refactored
IterStatto use generic stat types instead of boxed trait objects. - Bumped
numandordered-floatversions, fixed doc typos, and replaced a manualmatchwith the?operator.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rolling.rs | Made Rolling generic over U for static dispatch, preallocate VecDeque, adjusted error type |
| src/quantile.rs | Corrected reference titles and updated URLs in documentation |
| src/mean.rs | Simplified revert by using the ? operator instead of manual match |
| src/iter.rs | Changed IterStat to accept a generic stat type (U) instead of Box<dyn Univariate> |
| Cargo.toml | Bumped dependency versions for num and ordered-float |
| U: RollableUnivariate<F>, | ||
| F: Float + FromPrimitive + AddAssign + SubAssign, | ||
| { | ||
| fn update(&mut self, x: F) { |
There was a problem hiding this comment.
The update method never forwards the new value to to_roll.update(x) or pushes x onto window, so the sliding-window logic is broken; you should call self.to_roll.update(x) and self.window.push_back(x) after handling the oldest element.
src/rolling.rs
Outdated
| { | ||
| pub fn new(to_roll: &'a mut U, window_size: usize) -> Result<Self, &'static str> { // Optimization: &'static str for error (clearer, no lifetime tie) | ||
| if window_size == 0 { | ||
| return Err("Window size should not equals to 0"); |
There was a problem hiding this comment.
[nitpick] Grammar: change the error message to something like "Window size must be non-zero" or "Window size should not equal 0".
| return Err("Window size should not equals to 0"); | |
| return Err("Window size must be non-zero"); |
src/rolling.rs
Outdated
| /// let mut running_sum: Sum<f64> = Sum::new(); | ||
| /// // We wrap `running_sum` inside the `Rolling` struct. | ||
| /// let mut rolling_sum: Rolling<f64> = Rolling::new(& mut running_sum, 2).unwrap(); | ||
| /// let mut rolling_sum: Rolling<_, f64> = Rolling::new(& mut running_sum, 2).unwrap(); |
There was a problem hiding this comment.
[nitpick] Style: remove the extra space in & mut; it should be &mut running_sum to match Rust idioms.
| /// let mut rolling_sum: Rolling<_, f64> = Rolling::new(& mut running_sum, 2).unwrap(); | |
| /// let mut rolling_sum: Rolling<_, f64> = Rolling::new(&mut running_sum, 2).unwrap(); |
|
Great contribution, I will take a look on the code to review it asap 😄 |
|
Great MR, I'll review it asap, so far LGTM as test pass @OctopusTakopi @AdilZouitine |
Simple rolling sum bench: ~18% less compute time on my machine with 100000 data points and 100 window size |
AdilZouitine
left a comment
AdilZouitine
left a comment
There was a problem hiding this comment.
LGTM, flawless thank you 😄
chores:
perf: