chore: restructure transforms into modules #210
Conversation
WalkthroughThe pull request introduces a comprehensive refactoring of the data transformation module in the Changes
Sequence DiagramsequenceDiagram
participant User
participant Transform
participant Transformers
User->>Transform: Select transformation method
Transform->>Transformers: Apply transformation
Transformers-->>Transform: Return transformed data
Transform-->>User: Provide transformed iterator
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
crates/augurs-forecaster/src/transforms.rs (1)
64-68: Future exploration of a trait-based approach.These lines clarify why a trait-based design was deferred. This decision is justified due to complexity with trait objects and lifetimes. Revisiting it later might still be worthwhile for advanced composition, but the current enum approach is understandable.
crates/augurs-forecaster/src/transforms/power.rs (2)
6-17: Potential improvement for error management.Returning &'static str as an error for domain violations (e.g., x <= 0.0) might make error handling less flexible. Consider leveraging a common error type or a dedicated enum for domain violations to streamline error reporting and possibly preserve context about the invalid input value.
190-219: NaN fallback may mask user errors.When domain violations occur inside Box-Cox, the code replaces them with NaN in the iterator. Consider exposing the error or providing a logging/debug message so that potential data issues are not silently lost.
crates/augurs-forecaster/src/transforms/scale.rs (3)
57-69: Possible floating-point inaccuracies in scaling formula
When scaling large floating-point numbers or extremely small ones, floating-point precision could degrade. Consider documenting these numerical stability implications or enforcing constraints on ranges.
129-131: Export fields for StandardScaleParams with caution
Fields mean and std_dev are marked public. This is convenient but may allow external code to mutate them arbitrarily. If that’s unintentional, consider providing getters or restricting direct mutation.
155-179: Validate standard deviation edge cases
When the iterator is non-empty but has no variation (e.g., all samples are the same value), std_dev is zero. Your code returns zero, which is valid; subsequent scaling divides by zero. Ensure downstream usage handles infinite outputs or potential errors gracefully.crates/augurs-forecaster/src/transforms/exp.rs (3)
15-19: Enhance documentation for iterator adaptersWhile the implementation is solid, the documentation could be more comprehensive. Consider adding:
- Performance characteristics
- Usage examples
- Chaining examples with other iterators
Example documentation for Logit:
/// An iterator adapter that applies the logit function to each item. /// /// # Examples /// ``` /// use augurs_forecaster::transforms::LogitExt; /// /// let data = vec![0.5, 0.75, 0.25]; /// let transformed: Vec<_> = data.into_iter().logit().collect(); /// ``` /// /// # Performance /// This adapter is zero-cost and applies the transformation lazily.Also applies to: 42-46, 69-73, 96-100
123-192: Enhance test coverage with edge casesWhile the test suite is comprehensive for basic cases, consider adding tests for:
- Edge cases with very large/small numbers
- Numerical stability near boundaries
- Chaining multiple transformations
- Error cases (e.g., invalid inputs for logit)
Example additional test:
#[test] fn test_numerical_stability() { // Test logit near boundaries let x = 1e-10; let result = logit(x); assert!(!result.is_infinite()); // Test chaining transforms let data = vec![0.5, 0.75]; let result: Vec<_> = data.into_iter() .logit() .exp() .collect(); // Add assertions }
1-2: Consider adding module-level documentation about transformation systemAs part of the larger restructuring effort, it would be helpful to add module-level documentation explaining:
- How this module fits into the broader transformation system
- When to use each type of transformation
- Guidelines for implementing new transformations
Example module documentation:
//! Exponential transformations, including log and logit. //! //! This module is part of the transforms system and provides implementations //! for common exponential transformations. These transformations are useful //! for normalizing skewed data or transforming data to meet certain //! statistical assumptions. //! //! # Implementation Guidelines //! When adding new transformations: //! 1. Implement the core transformation function //! 2. Create an iterator adapter //! 3. Provide an extension trait //! 4. Add comprehensive tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/augurs-forecaster/src/lib.rs(0 hunks)crates/augurs-forecaster/src/power_transforms.rs(0 hunks)crates/augurs-forecaster/src/transforms.rs(2 hunks)crates/augurs-forecaster/src/transforms/exp.rs(1 hunks)crates/augurs-forecaster/src/transforms/power.rs(1 hunks)crates/augurs-forecaster/src/transforms/scale.rs(1 hunks)
💤 Files with no reviewable changes (2)
- crates/augurs-forecaster/src/lib.rs
- crates/augurs-forecaster/src/power_transforms.rs
🔇 Additional comments (13)
crates/augurs-forecaster/src/transforms.rs (4)
3-8: Documentation looks good.
These lines comprehensively describe the transformations module, including the usage and purpose of the Transform enum. Good job providing clear context for users.
11-13: Clear submodule structure.
Noting the presence of submodules in doc comments is helpful. Ensure these references stay updated as more transformations are introduced.
14-17: Good modular breakdown.
Splitting transformations into separate submodules ("exp", "power", "scale") helps maintain clarity and test them in isolation.
24-30: Well-organized imports.
Import statements and re-exports are neatly structured, making it easier to locate scaling and power transformation functionalities.
crates/augurs-forecaster/src/transforms/power.rs (7)
19-36: Appropriate handling of negative and nonnegative values.
The Yeo-Johnson transform checks for numeric validity and applies the correct formula based on whether the input is ≥ 0 or < 0. The logic is correct and handles special cases like lambda == 2.
38-66: Robust log-likelihood calculation for Box-Cox.
Error checks for empty data, nonpositive values, and zero variance ensure that invalid input is caught early. This helps mitigate subtle runtime errors.
68-105: Comprehensive error handling for Yeo-Johnson log-likelihood.
Similar to Box-Cox, these checks ensure the method fails gracefully when encountering invalid input or zero variance. Implementation is consistent across both transformations.
107-173: Effective approach to negative log-likelihood optimization.
Using Brent’s method with default bounds is sensible for searching the lambda space of Power transforms. The struct-based approach clarifies the optimization process.
223-235: Good domain validation for inverse Box-Cox.
Rejecting negative or zero domain values ensures consistent application of the transform. Keep in mind that returning an Err is helpful for debugging real-world data anomalies.
303-320: Thoughtful handling of edge cases for inverse Yeo-Johnson.
Using separate branches for lambda near 0 or 2 is crucial for numerical stability. This helps avoid NaN or infinities in borderline scenarios.
355-455: Extensive test coverage.
The test module covers both routine and edge cases for Box-Cox and Yeo-Johnson. These tests significantly reduce the risk of regressions and confirm correctness.
crates/augurs-forecaster/src/transforms/scale.rs (2)
250-361: Comprehensive and useful tests
The tests cover key scenarios such as typical data, empty data, single-value data, and custom scaling ranges. This is a strong practice, significantly enhancing confidence in correctness.
23-24: Clarify the usage of f64::EPSILON in scaled_min/max initialization
You are adding or subtracting EPSILON from 0.0 and 1.0, presumably to help avoid edge cases. However, this behavior may subtly shift the expected scaling range. Consider whether this shift is desired. If not, you might remove the epsilon or provide a documented rationale.
✅ Verification successful
Let me analyze the usage of EPSILON in the codebase to understand the context better.
The use of f64::EPSILON in scaled_min/max is justified and should be kept
The usage of f64::EPSILON in the min-max scaler implementation is intentional and correct. Here's why:
- The code implements a min-max scaler that maps input values to a range, defaulting to [0,1]
- The slight offset using EPSILON prevents numerical edge cases when the scaled values are used in subsequent calculations (like logarithms or divisions)
- The implementation is validated by comprehensive test cases that verify both the scaling and inverse scaling operations
- The behavior is consistent with common machine learning practices where exact boundary values (0 and 1) are sometimes avoided to prevent numerical instabilities
The tests demonstrate that this small epsilon adjustment doesn't impact the practical usage of the scaler while providing additional numerical robustness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usage of EPSILON in the code to confirm consistent approach.
rg --context 3 'EPSILON'
Length of output: 4491
Script:
#!/bin/bash
# Let's find the Scale struct and its implementation to understand the context
ast-grep --pattern 'struct Scale {
$$$
}'
Length of output: 45
Script:
#!/bin/bash
# Let's try to find the Scale implementation with ripgrep
rg -A 20 "struct Scale" crates/augurs-forecaster/src/transforms/scale.rs
Length of output: 74
Script:
#!/bin/bash
# Let's look at the entire scale.rs file to understand the full context
cat crates/augurs-forecaster/src/transforms/scale.rs
Length of output: 10631
| let (min, max) = data.fold((f64::INFINITY, f64::NEG_INFINITY), |(min, max), x| { | ||
| (min.min(x), max.max(x)) | ||
| }); | ||
| Self::new(min, max) | ||
| } |
There was a problem hiding this comment.
Potential issue with NaN handling in min/max calculation
Using min/max with NaN values typically results in NaN. The test at lines 354-361 implies that NaNs in the data are skipped or that min/max proceed as normal. However, Rust’s primitive f64::min and f64::max produce NaN if either operand is NaN. Consider using a filtering strategy or something like “.filter(|x| !x.is_nan())” to explicitly ignore NaNs.
pub fn from_data<T>(data: T) -> Self
where
T: Iterator<Item = f64>,
{
- let (min, max) = data.fold((f64::INFINITY, f64::NEG_INFINITY), |(min, max), x| {
- (min.min(x), max.max(x))
- });
+ let filtered_data = data.filter(|x| !x.is_nan());
+ let (min, max) = filtered_data.fold((f64::INFINITY, f64::NEG_INFINITY), |(mi, ma), x| {
+ (mi.min(x), ma.max(x))
+ });
Self::new(min, max)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (min, max) = data.fold((f64::INFINITY, f64::NEG_INFINITY), |(min, max), x| { | |
| (min.min(x), max.max(x)) | |
| }); | |
| Self::new(min, max) | |
| } | |
| pub fn from_data<T>(data: T) -> Self | |
| where | |
| T: Iterator<Item = f64>, | |
| { | |
| let filtered_data = data.filter(|x| !x.is_nan()); | |
| let (min, max) = filtered_data.fold((f64::INFINITY, f64::NEG_INFINITY), |(mi, ma), x| { | |
| (mi.min(x), ma.max(x)) | |
| }); | |
| Self::new(min, max) | |
| } |
| fn logit(x: f64) -> f64 { | ||
| (x / (1.0 - x)).ln() | ||
| } |
There was a problem hiding this comment.
Add input validation to prevent panics in logit function
The logit function will panic for inputs ≤ 0 or ≥ 1, and may have numerical stability issues near these boundaries. Consider adding input validation and handling edge cases.
Here's a suggested implementation:
fn logit(x: f64) -> f64 {
+ if x <= 0.0 || x >= 1.0 {
+ panic!("logit input must be between 0 and 1 exclusive");
+ }
+ // Add small epsilon to prevent numerical instability near boundaries
+ let x = x.clamp(1e-10, 1.0 - 1e-10);
(x / (1.0 - x)).ln()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn logit(x: f64) -> f64 { | |
| (x / (1.0 - x)).ln() | |
| } | |
| fn logit(x: f64) -> f64 { | |
| if x <= 0.0 || x >= 1.0 { | |
| panic!("logit input must be between 0 and 1 exclusive"); | |
| } | |
| // Add small epsilon to prevent numerical instability near boundaries | |
| let x = x.clamp(1e-10, 1.0 - 1e-10); | |
| (x / (1.0 - x)).ln() | |
| } |
The one transforms file was getting very long so this PR splits it up into submodules which is slightly easier to navigate and find things. Should be a no-op from a public API perspective.
Summary by CodeRabbit
New Features
Bug Fixes
power_transformsmodule and related functions, streamlining the codebase.Documentation
Tests