feat(js): expose MinMaxScaler transform
#237
Conversation
|
Warning Rate limit exceeded@yoziru has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 19 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Pipeline
participant Transformer
participant MinMaxScaler
User->>Pipeline: Create with MinMaxScaler
Pipeline->>Transformer: Convert Transform to Transformer
Transformer->>MinMaxScaler: Instantiate
MinMaxScaler-->>Transformer: Return scaler instance
Transformer-->>Pipeline: Configure pipeline
User->>Pipeline: Apply transformation
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/augurs-transforms-js/src/lib.rs (1)
17-18: Consider enhancing MinMaxScaler documentation and configuration.The documentation could be more specific about the default range and potential customization options. Additionally, consider adding
ignore_nansconfiguration like other transformers for consistent behavior.- /// Scale the data to a given range. + /// Scale the data to a specific range (default: [0, 1]). MinMaxScaler { + /// Whether to ignore NaNs. + #[serde(default, rename = "ignoreNaNs")] + ignore_nans: bool, + },js/testpkg/transforms.test.ts (2)
64-65: Consider fixing the type issue instead of using @ts-ignore.The @ts-ignore suggests a type mismatch between the custom matcher and the array types. Consider extending the TypeScript types properly:
declare global { namespace jest { interface Matchers<R> { toAllBeCloseTo(expected: number[]): R; } } }
84-90: Consider expanding NaN test cases.While the current tests cover basic NaN handling, consider adding edge cases:
- NaN at the start/end of array
- All NaN values
- Consecutive NaN values
// Additional test cases const allNaN = Array(y.length).fill(NaN); const consecutiveNaN = [...y]; consecutiveNaN[0] = NaN; consecutiveNaN[1] = NaN; testScalerPipeline('pipeline with all NaN', allNaN, 'standardScaler', true); testScalerPipeline('pipeline with consecutive NaN', consecutiveNaN, 'standardScaler', true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/augurs-transforms-js/src/lib.rs(3 hunks)js/testpkg/transforms.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (4)
js/augurs-transforms-js/src/lib.rs (2)
8-8: LGTM!The MinMaxScaler import is correctly added alongside other transformer imports.
36-38: LGTM!The MinMaxScaler transformer implementation follows the established pattern.
js/testpkg/transforms.test.ts (2)
43-48: Well-structured test helper function!Good application of DRY principle with type-safe parameters.
80-82: LGTM!Comprehensive test coverage for both scaler types.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/augurs-transforms-js/src/lib.rs (1)
17-18: Enhance MinMaxScaler variant documentation and configuration.Consider the following improvements:
- Expand the documentation to match the detail level of other variants
- Add
ignore_nansconfiguration like other scalers- Consider moving after StandardScaler for alphabetical ordering
- /// Scale the data to a given range. - MinMaxScaler, + /// Scale the data to a specified range (typically [0, 1]). + /// This transformation is useful when features have different scales + /// but do not make assumptions about the distribution. + MinMaxScaler { + /// Whether to ignore NaNs. + #[serde(default, rename = "ignoreNaNs")] + ignore_nans: bool, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/augurs-transforms-js/src/lib.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: linux (ubuntu-22.04, ppc64le)
- GitHub Check: linux (ubuntu-22.04, s390x)
- GitHub Check: linux (ubuntu-22.04, armv7)
- GitHub Check: linux (ubuntu-22.04, aarch64)
- GitHub Check: linux (ubuntu-22.04, x86)
- GitHub Check: musllinux (ubuntu-22.04, aarch64)
- GitHub Check: windows (windows-latest, x86)
- GitHub Check: Tests
- GitHub Check: Test Book
- GitHub Check: Clippy
- GitHub Check: windows (windows-latest, x64)
- GitHub Check: JS tests
- GitHub Check: Prophet WASMStan component
- GitHub Check: macos (macos-13, x86_64)
- GitHub Check: Check
🔇 Additional comments (2)
js/augurs-transforms-js/src/lib.rs (2)
8-8: LGTM! Import changes are well-organized.The MinMaxScaler import is correctly placed alongside other transformer imports.
17-18: Verify TypeScript type generation and documentation.Since this is a public API change, please ensure:
- TypeScript types are correctly generated
- Documentation includes JavaScript usage examples
- Breaking changes are documented if any
Also applies to: 36-36
✅ Verification successful
TypeScript generation infrastructure is in place
The codebase has proper TypeScript configuration and type definition generation setup. The new MinMaxScaler feature is documented in:
- crates/augurs-forecaster/README.md
- book/src/migrating.md
- book/src/getting-started/quick-start.md
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing usage and documentation rg -l "StandardScaler" --type md # Find documentation files rg -l "StandardScaler" --type ts # Find TypeScript filesLength of output: 65
Script:
#!/bin/bash # Search for MinMaxScaler usage and documentation rg -l "MinMaxScaler" --type-add 'docs:*.{md,rst,txt}' --type docs rg -l "MinMaxScaler" --type-add 'web:*.{js,ts,jsx,tsx}' --type web # Check for type generation configuration fd -e toml -e json -x cat {} \; # Look for cargo/package config fd "binding.gyp|tsconfig.json|package.json" -x cat {} \; # Look for JS/TS configLength of output: 65925
| @@ -31,6 +33,7 @@ pub enum Transform { | |||
| impl Transform { | |||
| fn into_transformer(self) -> Box<dyn Transformer> { | |||
| match self { | |||
| Transform::MinMaxScaler => Box::new(MinMaxScaler::new()), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add configuration handling for MinMaxScaler.
The current implementation doesn't handle ignore_nans and uses default range. Consider:
- Adding ignore_nans handling to match other transformers
- Adding feature_range configuration to allow custom scaling ranges
- Transform::MinMaxScaler => Box::new(MinMaxScaler::new()),
+ Transform::MinMaxScaler { ignore_nans } => {
+ Box::new(MinMaxScaler::new().ignore_nans(ignore_nans))
+ }Additionally, consider adding feature_range configuration to the enum variant and implementation if supported by the underlying MinMaxScaler:
MinMaxScaler {
#[serde(default, rename = "ignoreNaNs")]
ignore_nans: bool,
#[serde(default = "default_feature_range")]
feature_range: (f64, f64),
}Co-authored-by: Ben Sully <[email protected]>
Summary by CodeRabbit
New Features
MinMaxScalertransformation option to scale data within a specified range.Tests
Pipelineclass to use a more generalized testing function.Documentation
MinMaxScalertransformation.