feat!: allow ignoring NaNs in JS transforms, too #235
Conversation
Similar to #227, but for power transforms. Also add tests for ignoring NaNs in exponential transforms.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
participant User
participant Pipeline
participant Transform
participant Transformer
User->>Pipeline: Create with transforms
Pipeline->>Transform: Configure with ignoreNaNs
Transform-->>Transformer: Initialize with NaN handling
User->>Pipeline: Transform data
Pipeline->>Transformer: Apply transformation
Transformer-->>User: Return transformed data
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/augurs-transforms-js/src/lib.rs (1)
34-39: Consider standardizing the method names for consistency.The implementation uses different method names for similar functionality:
ignore_nans()for StandardScalerwith_ignore_nans()for YeoJohnsonConsider standardizing these method names across both transformers for better API consistency.
js/testpkg/transforms.test.ts (2)
67-71: Consider expanding NaN test cases.The test data setup is good but could be more comprehensive. Consider adding test cases for:
- NaNs at the start/end of the array
- Consecutive NaNs
- Edge cases with all NaNs
84-93: Consider optimizing test suite structure.The empty pipeline and invalid transform tests are duplicated from the main suite. Consider:
- Moving these to a shared test helper or using test.each
- Adding NaN-specific edge cases (e.g., testing transforms with mixed ignoreNaNs settings)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/augurs-transforms-js/src/lib.rs(1 hunks)js/testpkg/transforms.test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (3)
js/augurs-transforms-js/src/lib.rs (1)
14-28: LGTM! Well-documented enum changes.The Transform enum changes are well-structured with proper serde attributes and documentation. The breaking change is clearly implemented by adding the
ignore_nansfield to both transform variants.js/testpkg/transforms.test.ts (2)
45-45: LGTM! Test updated to use new transform format.The test correctly implements the breaking change by using objects with
typeproperty instead of strings.
72-82: LGTM! Comprehensive transform pipeline test with NaNs.The test properly verifies the transformation pipeline with NaN values, including both forward and inverse transformations.
It flows a bit nicer.
Unfortunately this requires a breaking change to the JS API; while transforms were previously specified as an array of strings, they are now specified as an array of objects with a `type` property. This is a breaking change, but it's a sensible one, because it will allow us to add further options to transforms in future. BREAKING CHANGE: transforms are now specified as an array of objects with a `type` property.
Unfortunately this requires a breaking change to the JS API;
while transforms were previously specified as an array of strings,
they are now specified as an array of objects with a
typeproperty.This is a breaking change, but it's a sensible one, because it will
allow us to add further options to transforms in future.
BREAKING CHANGE: transforms are now specified as an array of objects
with a
typeproperty.Summary by CodeRabbit
New Features
StandardScalerandYeoJohnsontransformationsTests