fix(rust): Improve error message for mismatched input lengths #22091
+42
−27
Conversation
This file contains 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
ydagosto
force-pushed
the
fix-corr-length-error
branch
2 times, most recently
from
9f93d63 to
c4d509a
Compare
ydagosto
changed the title
ydagosto
force-pushed
the
fix-corr-length-error
branch
4 times, most recently
from
b12533e to
a27d5b1
Compare
2 tasks
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22091 +/- ##
=======================================
Coverage 80.88% 80.89%
=======================================
Files 1641 1641
Lines 236509 236555 +46
Branches 2701 2705 +4
=======================================
+ Hits 191307 191351 +44
Misses 44563 44563
- Partials 639 641 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coastalwhite
requested changes
Apr 3, 2025
ydagosto
force-pushed
the
fix-corr-length-error
branch
3 times, most recently
from
567414c to
2f7122f
Compare
…remove redundant validation - Propagate input length validation error using `PolarsResult` in `cov` and `pearson_corr`. - Removed redundant validation inside `CovState::new()` and `PearsonState::new()` since validation is already performed at the API boundary.
ydagosto
force-pushed
the
fix-corr-length-error
branch
from
2f7122f to
97db07e
Compare
@coastalwhite - error should now be propagated all the way. Also removed some assertions that seemed redundant. Let me know what you think |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes: #22080
Overview
This PR improves the error handling in corr and pearson_corr functions by replacing a panic assertion with a descriptive PolarsError::ComputeError.
Previously, when input Series to corr had mismatched lengths, Polars would panic with an unhelpful Rust assertion error.
This PR introduces a proper ComputeError with a clear and actionable message:
"Input Series to corr must have the same length, but got lengths {} and {}."
This improves the developer experience and allows for cleaner error propagation in the future.
Ouptut log
Notes
Removed redundant input length validation in CovState::new() and PearsonState::new() since the check is already performed in cov() and pearson_corr().