Add check_cudf, and a few other validation changes - #8038
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded index coercion utilities and a new public check_cudf function for cudf-aware coercion and shape validation; refactored check_array to handle cudf dtype fallbacks; extended check_y with ensure_discrete_classes and return_index options and updated class extraction and return signatures to include coerced indices. ChangesCore validation utilities
Test coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/internals/validation.py`:
- Around line 844-848: Add stacklevel=2 to the warnings.warn calls in the
validation helpers so the warning reports the caller's location rather than this
internal function; update the warnings.warn invocations shown (the column-vector
message and the other similar warn at lines ~855-859) to pass stacklevel=2
(i.e., warnings.warn(..., DataConversionWarning, stacklevel=2)) so callers of
check_cudf and related validation functions receive correctly attributed
warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a7bee1e1-8b05-42fc-93a1-35c96c7ec2d7
📒 Files selected for processing (2)
python/cuml/cuml/internals/validation.pypython/cuml/tests/test_validation.py
This is staging some validation functionality needed to cleanup and improve our preprocessing encoders. I've split out the validation changes into a separate PR (with tests) to ease review. The main changes here are: - Addition of a new `check_cudf` function. This validates and coerces user input into a `cudf` object (a `DataFrame`, `Series`, or either depending on dimensionality), following the same general validation flow as `check_array`. This is useful for estimators that make use of `cudf` functionality, or those that need to work with non-numeric types (like our encoders). The checks and knobs are pared down to just what is needed for these estimators, but all error messages and checks are compatible with what sklearn expects. - Addition of a `return_index` option to `check_y`. In rare occasions we sometimes want to return an output that aligns with the index of `y`, and having a way to strip that off in `check_y` is useful. - Addition of `ensure_discrete_classes` to `check_y`. Setting this to `False` disables the continuous value check used when encoding class labels. These will be used in follow-up PRs to simplify several preprocessing estimators, as well as metrics and other label-handling code.
c082073 to
37cd068
Compare
|
Thanks for the review! I believe all comments have been addressed. |
|
/merge |
This cleans up our `LabelEncoder` implementation, preparing for exposing it through `cuml.accel`. This one needed a lot of work, and there are _several breaking changes_. - The type of `LabelEncoder.classes_` is now a `numpy.ndarray` instead of a `cudf.Series`. Note that `classes_` was undocumented. This mirrors the `classes_` attribute in all our other classifiers (all of which also made this as a breaking change many releases ago). I view this in the same vein. - Likewise, since `OneHotEncoder` and `OrdinalEncoder` make use of `LabelEncoder` internally, the type of `categories_` has changed there from `list[cudf.Series]` to `list[numpy.ndarray]`. This now better matches with sklearn, and makes sense IMO in the same way that changing `classes_` above did. - `LabelEncoder` now follows our type-reflection standards, previously it always returned `cudf` objects (even though the docstrings implied type-reflection was followed). I view this as a bugfix. - `LabelEncoder.inverse_transform` now handles `handle_unknown="ignore"` _as documented_. Previously this would return incorrect values in the presence of missing values. Note that `handle_unknown` is _not_ an argument to `sklearn.preprocessing.LabelEncoder`. I view this as a bugfix. This required munging in our tests and the implementations of `OrdinalEncoder`/`OneHotEncoder` (and their dask counterparts), but beyond the changes listed above should have no other user-facing ramifications. Stacked on top of #8038. Part of #7317. Precursor for #8015. Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Simon Adorf (https://github.com/csadorf) URL: #8039
This is staging some validation functionality needed to cleanup and improve our preprocessing encoders. I've split out the validation changes into a separate PR (with tests) to ease review.
The main changes here are:
check_cudffunction. This validates and coerces user input into acudfobject (aDataFrame,Series, or either depending on dimensionality), following the same general validation flow ascheck_array. This is useful for estimators that make use ofcudffunctionality, or those that need to work with non-numeric types (like our encoders). The checks and knobs are pared down to just what is needed for these estimators, but all error messages and checks are compatible with what sklearn expects.return_indexoption tocheck_y. In rare occasions we sometimes want to return an output that aligns with the index ofy, and having a way to strip that off incheck_yis useful.ensure_discrete_classestocheck_y. Setting this toFalsedisables the continuous value check used when encoding class labels.These will be used in follow-up PRs to simplify several preprocessing estimators, as well as metrics and other label-handling code.