Apply new validation to metrics.kl_divergence - #8057
Merged
rapids-bot[bot] merged 1 commit intoMay 6, 2026
Merged
Conversation
Replace input_to_cuml_array in cuml.metrics.kl_divergence with check_array from cuml.internals.validation, matching the pattern established by the prior cuml.metrics migrations (xref NVIDIA#7998). Accepts both (N,) and (N, 1) shapes and rejects 2D inputs with multiple columns explicitly. The dtype-mismatch error path now raises ValueError (sklearn-aligned) instead of TypeError; test_kl_divergence updated accordingly.
csadorf
added
improvement
📝 WalkthroughSummary by CodeRabbit
WalkthroughKL divergence metric implementation in Cython migrated from ChangesKL Divergence Input Validation Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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.
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/metrics/kl_divergence.pyx (1)
47-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring is inaccurate about dtype conversion behavior.
The docstring states that
convert_dtype=Truewill convert bothPandQtofloat32, but the actual implementation accepts bothfloat32andfloat64(line 64) and convertsQto matchP's dtype (line 80). The conversion target depends onP's dtype, not a fixedfloat32.📝 Suggested docstring fix
convert_dtype : bool, optional (default = True) - When set to True, the method will, convert P and - Q to be the same data type: float32. This - will increase memory used for the method. + When set to True, the method will convert Q to + match the data type of P (float32 or float64). This + may increase memory used for the method.🤖 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/metrics/kl_divergence.pyx` around lines 47 - 50, The docstring for the kl_divergence function incorrectly states convert_dtype converts both P and Q to float32; update it to say that when convert_dtype=True, P and Q are converted to the same floating dtype (float32 or float64) supported by the implementation, with Q being cast to match P.dtype (i.e., the target dtype follows P's dtype), and mention supported dtypes are float32 and float64; ensure the docstring references the parameters convert_dtype, P, and Q and accurately reflects the actual conversion behavior.
🧹 Nitpick comments (1)
python/cuml/cuml/metrics/kl_divergence.pyx (1)
91-91: 💤 Low valueVariable name
n_features_pis misleading.This variable represents the number of elements in the probability distribution (i.e.,
n_samples), not the number of features. For KL divergence, P and Q are 1D probability vectors, son_samplesorn_elementswould be clearer.♻️ Suggested rename
- cdef int n_features_p = P_m.shape[0] - if Q_m.shape[0] != n_features_p: + cdef int n_samples = P_m.shape[0] + if Q_m.shape[0] != n_samples: raise ValueError( "Incompatible dimension for P and Q arrays: " - f"P.shape == ({n_features_p},) while Q.shape == ({Q_m.shape[0]},)" + f"P.shape == ({n_samples},) while Q.shape == ({Q_m.shape[0]},)" ) - cdef uintptr_t d_P_ptr = P_m.data.ptr - cdef uintptr_t d_Q_ptr = Q_m.data.ptr + cdef uintptr_t d_P_ptr = P_m.data.ptr + cdef uintptr_t d_Q_ptr = Q_m.data.ptr if (dtype_p == np.float32): res = c_kl_divergence(handle_[0], <float*> d_P_ptr, <float*> d_Q_ptr, - n_features_p) + n_samples) else: res = c_kl_divergence(handle_[0], <double*> d_P_ptr, <double*> d_Q_ptr, - n_features_p) + n_samples)🤖 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/metrics/kl_divergence.pyx` at line 91, Rename the misleading variable n_features_p to a clearer name like n_samples (or n_elements) in this KL divergence implementation: update the declaration cdef int n_features_p = P_m.shape[0] and replace all subsequent uses of n_features_p throughout the function (and any helper variables/loops that reference it) to the new identifier so it reflects that P_m and Q_m are 1D probability vectors; ensure consistency for related symbols (e.g., if there are n_features_q or similar) and run tests to confirm no remaining references to n_features_p.
🤖 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.
Outside diff comments:
In `@python/cuml/cuml/metrics/kl_divergence.pyx`:
- Around line 47-50: The docstring for the kl_divergence function incorrectly
states convert_dtype converts both P and Q to float32; update it to say that
when convert_dtype=True, P and Q are converted to the same floating dtype
(float32 or float64) supported by the implementation, with Q being cast to match
P.dtype (i.e., the target dtype follows P's dtype), and mention supported dtypes
are float32 and float64; ensure the docstring references the parameters
convert_dtype, P, and Q and accurately reflects the actual conversion behavior.
---
Nitpick comments:
In `@python/cuml/cuml/metrics/kl_divergence.pyx`:
- Line 91: Rename the misleading variable n_features_p to a clearer name like
n_samples (or n_elements) in this KL divergence implementation: update the
declaration cdef int n_features_p = P_m.shape[0] and replace all subsequent uses
of n_features_p throughout the function (and any helper variables/loops that
reference it) to the new identifier so it reflects that P_m and Q_m are 1D
probability vectors; ensure consistency for related symbols (e.g., if there are
n_features_q or similar) and run tests to confirm no remaining references to
n_features_p.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: de4414b0-2fdd-475c-9105-d4eb8e5967f5
📒 Files selected for processing (2)
python/cuml/cuml/metrics/kl_divergence.pyxpython/cuml/tests/test_metrics.py
csadorf
added
improvement
Member
|
/merge |
csadorf
deleted the
issue-7998-apply-new-validation-to-metrics-kl-divergence
branch
May 6, 2026 21:31
This was referenced May 7, 2026
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.
Apply new input validation to
cuml.metrics.kl_divergence.Part of #7998.