Cleanup and apply new validation to cuml.naive_bayes - #8051
Merged
Conversation
jcrist
added
improvement
|
Caution Review failedFailed to post review comments 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds public check_classification_targets and extends check_y/check_inputs to support explicit class returns (single- and multi-output) and index propagation. Refactors Naive Bayes into a shared _BaseNB with unified counting, prediction/probability plumbing, and extensive categorical/sparse counting changes. Tests and sklearn-compat mappings updated. ChangesValidation + Naive Bayes refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuml/cuml/naive_bayes/naive_bayes.py (1)
1296-1315:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
_count_sparse()for all-zero sparse batches.
x_coo_data.max()raises onnnz == 0, soCategoricalNB.fit/partial_fitwill crash on a valid sparse batch whose entries are all zero. For this estimator, zero is a legitimate category and should be handled the same way as the dense path.Proposed fix
- highest_feature = int(x_coo_data.max()) + 1 + highest_feature = ( + int(x_coo_data.max()) + 1 if x_coo_data.size else 1 + )As per coding guidelines,
behavior for edge cases (empty arrays, single sample) must match scikit-learn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuml/cuml/naive_bayes/naive_bayes.py` around lines 1296 - 1315, The code calls x_coo_data.max() unguarded which throws when nnz == 0; update _count_sparse to handle all-zero sparse batches by checking x_coo_nnz (or len(x_coo_data)) first and if it is 0 set highest_feature = 1 (so category 0 is preserved and behavior matches the dense path), otherwise compute highest_feature = int(x_coo_data.max()) + 1; reference the _count_sparse function and the x_coo_data / x_coo_nnz / highest_feature symbols when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuml/cuml/naive_bayes/naive_bayes.py`:
- Around line 162-169: predict() currently ignores the index returned by
_check_predict(X, ...) and returns decode_labels(...) directly, dropping
DataFrame/cuDF indices; update predict (in naive_bayes.NaiveBayes.predict) to
preserve input labels the same way predict_proba()/predict_log_proba() do by
capturing the index from _check_predict, calling decode_labels to get the
predicted labels, and then wrapping those labels in a CumlArray (or the same
array wrapper used by predict_proba/predict_log_proba) with index=index and
output_type=output_type before returning; ensure you use the same
constructor/signature (e.g., CumlArray(..., index=index)) and keep
decode_labels, _check_predict, and _get_output_type references intact.
- Around line 621-627: Change the public signatures of the estimator methods to
keep sample_weight=None (e.g., def partial_fit(..., sample_weight=None) and def
fit(..., sample_weight=None)) so they remain scikit-learn compatible; if you
need to mark the parameter deprecated, handle deprecation inside the method
bodies of partial_fit and fit by checking for a private sentinel (or inspecting
sample_weight) and emitting a deprecation warning or mapping to the internal
sentinel, rather than changing the default in the signature of partial_fit, fit,
or any other estimator methods in _BaseDiscreteNB.
---
Outside diff comments:
In `@python/cuml/cuml/naive_bayes/naive_bayes.py`:
- Around line 1296-1315: The code calls x_coo_data.max() unguarded which throws
when nnz == 0; update _count_sparse to handle all-zero sparse batches by
checking x_coo_nnz (or len(x_coo_data)) first and if it is 0 set highest_feature
= 1 (so category 0 is preserved and behavior matches the dense path), otherwise
compute highest_feature = int(x_coo_data.max()) + 1; reference the _count_sparse
function and the x_coo_data / x_coo_nnz / highest_feature symbols when applying
the change.
🪄 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: b9abd431-3314-4765-9e2b-73416c6c0597
📒 Files selected for processing (6)
python/cuml/cuml/internals/validation.pypython/cuml/cuml/naive_bayes/naive_bayes.pypython/cuml/tests/test_base.pypython/cuml/tests/test_naive_bayes.pypython/cuml/tests/test_sklearn_compatibility.pypython/cuml/tests/test_validation.py
This adds support to `check_y`/`check_inputs` for explicitly specifying the classes used for label encoding via the `return_classes` keyword. This may optionally take a numpy array (or list of numpy arrays) in the same format as those returned from `check_y`. If provided, these will be used for label encoding instead of deriving the classes from `y` itself. If any unknown classes are found in the data, a useful error will be raised.
This is a general cleanup of `cuml.naive_bayes`, motivated _mostly_ by the new validation work. It: - Ports `cuml.naive_bayes` to use the new validation utilities, fixing many xfails. - Adds support for returning non-numeric classes from all classifiers, mirroring our support in our other classifiers. - Moves the `classes_` attribute to be a numpy array (technically a breaking change), mirroring how it's handled on every other classifier. - Simplifies the control flow and definitions, greatly reducing the LOC in this module.
This argument was previously unsupported and was just silently ignored (leading to incorrect results). We now raise a `NotImplementedError`, but even having the arg at all leads to some failing tests in the sklearn test suite. Since we don't intend to spend time adding support for `sample_weight`, better to just deprecate and rip it out instead.
jcrist
force-pushed
the
validation-naive-bayes
branch
from
May 6, 2026 17:13
bd02b4f to
ccc9b6e
Compare
Member
Author
|
|
csadorf
approved these changes
May 6, 2026
Contributor
|
/merge |
Merged
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This is a general cleanup of
cuml.naive_bayes, as well as applying the new validation utilities.This:
cuml.naive_bayesto use the new validation utilities, fixing many xfails.classes_attribute to be a numpy array (a breaking change), mirroring how it's handled on every other classifier.NotImplementedErrorifsample_weightis provided toBernouilliNB/CategoricalNB/MultinomialNB/ComplementNB. This argument was never supported, but would previously be silently ignored (leading to incorrect results!). Viewing this as a bug, I added an error so users wouldn't mistakenly think their weights were being applied.sample_weighttoBernoulliNB/CategoricalNB/MultinomialNB/ComplementNB. Given the above, it doesn't make sense to keep around a parameter that we don't actually support (and silently ignoring can give wrong results). This deprecates it in 26.06, slated for full removal in 26.08. We warn if any value is passed viasample_weight(even supported things likeNone), and only error for unsupported things like arrays of weights.To accomplish this, I also added support for specifying the classes to use for label encoding explicitly to
check_y/check_inputsvia thereturn_classeskeyword. This may optionally take a numpy array (or list of numpy arrays) in the same format as those returned fromcheck_y. If provided, these will be used for label encoding instead of deriving the classes fromyitself. If any unknown classes are found in the data, a useful error will be raised. This is useful for classifiers that supportpartial_fit(like those innaive_bayes) where you want to consistently label encodeyacross multiplepartial_fitcalls.Fixes #8000.
Fixes #6228.