Deprecate convert_dtype - #8300
Conversation
jcrist
left a comment
There was a problem hiding this comment.
Annotating the diff for some notable locations. All other files are mechanical changes.
| @@ -538,7 +538,7 @@ def check_array( | |||
| accept_sparse=False, | |||
There was a problem hiding this comment.
This file implements the deprecation.
| assert ptr(res.indptr) != ptr(array.indptr) | ||
|
|
||
|
|
||
| def test_convert_dtype_deprecated(): |
There was a problem hiding this comment.
Here we test the deprecation.
| "convert_dtype_single": "convert_dtype : bool, optional (default = {default})\n" | ||
| " When set to True, the method will automatically\n" | ||
| " convert the inputs to {dtype}.", | ||
| "convert_dtype": "convert_dtype : bool, optional (default = 'deprecated')\n" |
There was a problem hiding this comment.
Here we update the docstring generator for documenting the deprecation of convert_dtype.
I also slightly updated the dtype notes on other arg types. In the future I'd like to do more cleanups here (regarding how we document other parameters), was only trying to do the minimal amount so things weren't definitely incorrect.
📝 WalkthroughSummary by CodeRabbit
WalkthroughDeprecates ChangesValidation, docstrings, and API defaults
Test updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
python/cuml/cuml/internals/validation.py (2)
1354-1370: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH:
check_sample_weightonly warns on array-like inputs.
Noneand scalarsample_weightvalues return beforecheck_array(...), socheck_sample_weight(None, convert_dtype=True)and scalar calls silently bypass the deprecation. That leaves a public helper with input-dependent warning behavior.Proposed fix
def check_sample_weight( sample_weight, *, dtype=None, convert_dtype="deprecated", @@ ): """Validate and coerce ``sample_weight`` to a supported type. @@ """ + if convert_dtype != "deprecated": + warnings.warn( + "`convert_dtype` was deprecated in version 26.08 and will be " + "removed in version 26.10. cuML only copies input arrays when " + "necessary (e.g. to unify dtypes), there is no reason to " + "provide this keyword going forward.", + FutureWarning, + ) + if sample_weight is None: return None🤖 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/internals/validation.py` around lines 1354 - 1370, `check_sample_weight` has input-dependent deprecation behavior because `None` and scalar values return before `check_array(...)`, so they skip the `convert_dtype` warning path. Update `check_sample_weight` so the deprecation check runs for all inputs, including `None` and scalar `sample_weight`, by moving or duplicating the `convert_dtype` handling ahead of the early returns. Keep the existing validation logic in `check_sample_weight` and `check_array`/`validate_sample_weight` flow consistent for array-like, scalar, and `None` inputs.Source: Coding guidelines
1131-1169: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH:
check_y(..., return_classes=True)skips theconvert_dtypedeprecation warning.This branch never emits the new warning: it omits
convert_dtypefrom the internalcheck_array(...)call, socheck_y(y, return_classes=True, convert_dtype=True/False)is silent while thereturn_classes=Falsepath warns. That makes the helper’s deprecation behavior branch-dependent and the new test won’t catch it because it only exercises the non-return_classespath.Proposed fix
def check_y( y, *, dtype=None, convert_dtype="deprecated", @@ ): """Validate and coerce ``y`` to a supported type. @@ """ + if convert_dtype != "deprecated": + warnings.warn( + "`convert_dtype` was deprecated in version 26.08 and will be " + "removed in version 26.10. cuML only copies input arrays when " + "necessary (e.g. to unify dtypes), there is no reason to " + "provide this keyword going forward.", + FutureWarning, + ) + if y is None: raise ValueError( "This estimator requires y to be passed, but the target y is None" )🤖 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/internals/validation.py` around lines 1131 - 1169, The `check_y(..., return_classes=True)` branch is missing the `convert_dtype` deprecation warning that the non-`return_classes` path already emits. Update `check_y` so the `return_classes` flow also routes through the same deprecation handling around the internal `check_array(...)` call, using the existing `convert_dtype` argument and warning logic consistently in both branches.Source: Coding guidelines
python/cuml/tests/test_logistic_regression.py (1)
474-482: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHIGH:
test_logistic_predict_output_dtypeno longer checks output dtype.After the rename, this still only verifies that
predict()does not raise. A regression in the returned dtype/type would pass unnoticed, so the test should assert the expected prediction dtype/type explicitly (or keep the old smoke-test-oriented name). As per coding guidelines, HIGH test issues include cases that only check “runs without error” inpython/**tests.🤖 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/tests/test_logistic_regression.py` around lines 474 - 482, `test_logistic_predict_output_dtype` currently only smoke-tests `cuLog.predict()` and does not validate the returned dtype/type. Update this test to capture the result of `clf.predict(X_test.astype(test_dtype))` and assert the expected prediction dtype or array type explicitly, using the existing `dataset`, `test_dtype`, and `cuLog` setup; if you intentionally want a smoke test, rename the test to reflect that behavior instead.Source: Coding guidelines
python/cuml/cuml/metrics/pairwise_distances.pyx (1)
270-303: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: Forward
convert_dtypethrough thenan_euclideandispatch.Changing this parameter to the deprecation sentinel exposes that
pairwise_distances(..., metric="nan_euclidean", convert_dtype=False)still drops the caller-supplied value at the early return. That changes behavior before the advertised removal window and skips the warning path entirely.As per coding guidelines, Python API changes should preserve backward-compatible behavior throughout the deprecation cycle.
Proposed fix
- if metric == "nan_euclidean": - return nan_euclidean_distances(X, Y, **kwds) + if metric == "nan_euclidean": + return nan_euclidean_distances( + X, Y, convert_dtype=convert_dtype, **kwds + )🤖 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/pairwise_distances.pyx` around lines 270 - 303, The early-return path in pairwise_distances for metric="nan_euclidean" is dropping the caller’s convert_dtype value, which bypasses the existing deprecation behavior. Update the pairwise_distances dispatch so the nan_euclidean branch still forwards convert_dtype into the downstream implementation instead of replacing it with the deprecation sentinel. Make the change in the pairwise_distances function while preserving the current warning/deprecation flow and backward-compatible behavior.Source: Coding guidelines
python/cuml/cuml/metrics/pairwise_kernels.py (1)
181-187: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH:
pairwise_kernelswon't emit the advertisedconvert_dtypedeprecation.This signature/docstring now deprecates
convert_dtype, but the implementation still callscheck_array()with its own default and never forwards the caller's value. As written,pairwise_kernels(..., convert_dtype=...)will skip the sharedFutureWarningpath entirely, so users get no notice before the kwarg is removed.Suggested fix
- X = check_array(X, input_name="X") + X = check_array(X, input_name="X", convert_dtype=convert_dtype)Also applies to: 227-233
🤖 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/pairwise_kernels.py` around lines 181 - 187, `pairwise_kernels` is not forwarding the deprecated `convert_dtype` argument into the shared input validation path, so the expected deprecation warning never fires. Update `pairwise_kernels` to pass the caller-provided `convert_dtype` through to `check_array()` (and any related validation helper used before kernel computation), and keep the handling consistent with the other metric wrappers so the FutureWarning is emitted whenever this kwarg is used.python/cuml/cuml/cluster/kmeans.pyx (1)
805-813: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH:
fit_transformstill ignoresconvert_dtypeduring the fit step.
fit_transform(..., convert_dtype=...)callsself.fit(X, sample_weight=sample_weight)without forwarding the kwarg, so the argument is only honored fortransform. That can silently coerce duringfitand then fail duringtransform, leaving the estimator fitted afterfit_transformraises.Proposed fix
def fit_transform( self, X, y=None, sample_weight=None, *, convert_dtype="deprecated" ) -> CumlArray: """ Compute clustering and transform X to cluster-distance space. """ - self.fit(X, sample_weight=sample_weight) + self.fit( + X, + sample_weight=sample_weight, + convert_dtype=convert_dtype, + ) return self.transform(X, convert_dtype=convert_dtype)🤖 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/cluster/kmeans.pyx` around lines 805 - 813, The `KMeans.fit_transform` path is not forwarding `convert_dtype` into the fit step, so the kwarg is only applied in `transform`. Update `fit_transform` to pass `convert_dtype` through to `self.fit(...)` as well as `self.transform(...)`, matching the existing `fit`/`transform` behavior and ensuring dtype handling is consistent across the full call.python/cuml/cuml/decomposition/incremental_pca.py (2)
205-224: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHIGH:
fit()exposesconvert_dtypewithout documenting the deprecation.The public signature now includes
convert_dtype="deprecated", but the docstring still documents onlyXandy. That leaves the published API docs inconsistent withtransform()and hides the removal timeline from callers.📝 Suggested doc update
Parameters ---------- X : array-like or sparse matrix, shape (n_samples, n_features) Training data, where n_samples is the number of samples and n_features is the number of features. y : Ignored + convert_dtype : bool, default="deprecated" + .. deprecated:: 26.08 + `convert_dtype` was deprecated in version 26.08 and will be + removed in version 26.10. cuML only copies input arrays when + necessary (e.g. to unify dtypes), there is no reason to provide + this keyword going forward.As per coding guidelines, "Missing or incorrect docstrings for public methods" is a HIGH issue.
🤖 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/decomposition/incremental_pca.py` around lines 205 - 224, Update the IncrementalPCA.fit docstring to document the convert_dtype keyword and its deprecation status so the public API matches the signature. In the fit method, add a Parameters entry for convert_dtype and note that it is deprecated, aligning the wording with the existing transform() documentation and making the deprecation/removal timeline explicit for callers.Source: Coding guidelines
401-455: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: Dense
transform()silently drops the caller'sconvert_dtype.The sparse branch forwards
convert_dtypeintocheck_inputs, but the dense branch callssuper().transform(X)without it. For dense inputs, an explicitconvert_dtype=False/Truetherefore never reaches the validation layer, so the deprecation warning and any non-default behavior are skipped only for that path.🔧 Suggested fix
- return super().transform(X) + return super().transform(X, convert_dtype=convert_dtype)🤖 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/decomposition/incremental_pca.py` around lines 401 - 455, The dense path in IncrementalPCA.transform is dropping the caller-provided convert_dtype value, so the behavior differs from the sparse branch. Update transform so the explicit convert_dtype argument is forwarded through the dense branch as well, using the existing super().transform path in a way that preserves this parameter. Keep the sparse branch’s check_inputs flow unchanged, and ensure the deprecation behavior for convert_dtype is exercised consistently for both dense and sparse inputs.python/cuml/cuml/metrics/cluster/silhouette_score.pyx (1)
143-179: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHIGH: Document
convert_dtypeon the silhouette public wrappers.Issue: both wrappers accept
convert_dtype="deprecated"but their docstrings omit the deprecation/removal notice. Impact: users won’t get consistent migration guidance across metrics APIs.As per coding guidelines, “Missing or incorrect docstrings for public methods” are high issues.
Proposed docstring additions
chunksize : integer (default = None) An integer, 1 <= chunksize <= n_samples to tile the pairwise distance matrix computations, so as to reduce the quadratic memory usage of having the entire pairwise distance matrix in GPU memory. If None, chunksize will automatically be set to 40000, which through experiments has proved to be a safe number for the computation to run on a GPU with 16 GB VRAM. + convert_dtype : bool, default="deprecated" + .. deprecated:: 26.08 + `convert_dtype` was deprecated in version 26.08 and will be + removed in version 26.10. cuML only copies input arrays when + necessary (e.g. to unify dtypes), there is no reason to provide + this keyword going forward.Apply this section to both
cython_silhouette_scoreandcython_silhouette_samples.Also applies to: 182-220
🤖 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/cluster/silhouette_score.pyx` around lines 143 - 179, The public silhouette wrappers `cython_silhouette_score` and `cython_silhouette_samples` accept `convert_dtype="deprecated"` but their docstrings do not mention the deprecation or removal guidance. Update both docstrings to document `convert_dtype` with a clear deprecation notice and migration guidance so the public API matches the other metrics wrappers and users see consistent guidance.Source: Coding guidelines
python/cuml/cuml/metrics/cluster/adjusted_rand_index.pyx (1)
22-37: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHIGH: Add the
convert_dtypedeprecation notice to the public docstring.Issue: the public signature exposes
convert_dtype="deprecated", but the docstring omits the parameter and removal timeline. Impact: users won’t see deprecation guidance for this metrics API.As per coding guidelines, “Missing or incorrect docstrings for public methods” are high issues.
Proposed docstring addition
labels_pred : Array of predicted labels used to evaluate the model + convert_dtype : bool, default="deprecated" + .. deprecated:: 26.08 + `convert_dtype` was deprecated in version 26.08 and will be + removed in version 26.10. cuML only copies input arrays when + necessary (e.g. to unify dtypes), there is no reason to provide + this keyword going forward. + Returns🤖 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/cluster/adjusted_rand_index.pyx` around lines 22 - 37, Update the public docstring for adjusted_rand_score to document the convert_dtype argument as deprecated, including the deprecation notice and removal timeline. Make sure the Parameters section mentions convert_dtype alongside labels_true and labels_pred, and reference the adjusted_rand_score signature so the docstring matches the exposed API. Keep the guidance explicit that this parameter is deprecated and should not be used in new code.Source: Coding guidelines
python/cuml/cuml/dask/neighbors/kneighbors_classifier.py (1)
280-299: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHIGH: Document
score’s deprecatedconvert_dtypeparameter.Issue:
scorestill accepts the changed public keyword but the docstring omits the deprecation/removal notice. Impact: users callingscore(..., convert_dtype=...)do not get API migration guidance from the docs.As per coding guidelines, “Missing or incorrect docstrings for public methods” are high issues.
Proposed docstring addition
y : array-like (device or host) shape = (n_samples, n_features) Labels test data. Acceptable formats: dask CuPy/NumPy/Numba Array + convert_dtype : bool, default="deprecated" + .. deprecated:: 26.08 + `convert_dtype` was deprecated in version 26.08 and will be + removed in version 26.10. cuML only copies input arrays when + necessary (e.g. to unify dtypes), there is no reason to provide + this keyword going forward. + Returns -------🤖 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/dask/neighbors/kneighbors_classifier.py` around lines 280 - 299, The public method score in kneighbors_classifier.KNeighborsClassifier still accepts convert_dtype but its docstring does not mention that the parameter is deprecated. Update the score docstring to document convert_dtype as a deprecated keyword, including a brief deprecation/removal note in the Parameters section so users know how to migrate; keep the change localized to the score method documentation.Source: Coding guidelines
python/cuml/cuml/neighbors/nearest_neighbors.pyx (1)
705-727: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: Sparse
kneighborsstill drops explicitconvert_dtypevalues.
kneighbors()now advertises the deprecation contract, but the sparse path still never forwards the caller’sconvert_dtypeintocheck_array. That meansconvert_dtype=Trueon CSR input will not emit the newFutureWarning, andconvert_dtype=Falseis still silently ignored for sparse queries.Suggested fix
- distances_cp, indices_cp = self._kneighbors_sparse(X, n_neighbors) + distances_cp, indices_cp = self._kneighbors_sparse( + X, n_neighbors, convert_dtype=convert_dtype + )- def _kneighbors_sparse(self, X, int n_neighbors): + def _kneighbors_sparse( + self, X, int n_neighbors, *, convert_dtype="deprecated" + ): if not is_sparse(X): raise ValueError("A NearestNeighbors model trained on sparse " "data requires sparse input to kneighbors()") @@ X_cp = check_array( X, dtype="float32", accept_sparse=["csr"], input_name="X", + convert_dtype=convert_dtype, )Also applies to: 901-905
🤖 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/neighbors/nearest_neighbors.pyx` around lines 705 - 727, The sparse kneighbors path in nearest_neighbors.pyx is still ignoring the caller’s explicit convert_dtype value, so update the sparse-input handling in kneighbors() to pass convert_dtype through to check_array just like the dense path. Make sure the same fix is applied wherever the sparse branch is handled so explicit convert_dtype=True/False behavior is preserved and the deprecation warning contract is triggered consistently.python/cuml/cuml/svm/svc.py (1)
417-437: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: Multiclass SVC inference bypasses the new
convert_dtypecontract.Both multiclass branches drop the caller’s
convert_dtypeby delegating toself._multiclass.*(X)directly. So binary SVC will still warn on legacyconvert_dtype=True/False, but multiclass SVC will not, andFalseis effectively ignored on those paths.Also applies to: 448-460
🤖 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/svm/svc.py` around lines 417 - 437, The multiclass path in SVC.predict is bypassing the new convert_dtype behavior, so the _multiclass branch should be updated to honor the caller’s convert_dtype argument just like the binary path does. Adjust the multiclass delegation in predict (and any shared multiclass helper it calls) so legacy convert_dtype values still trigger the expected warning/handling and are not silently ignored, while keeping decode_labels and _get_output_type unchanged.python/cuml/cuml/dask/manifold/umap.py (1)
90-112: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHIGH:
UMAP.transformstill omits the deprecatedconvert_dtypeparameter from its docs.The signature now exposes
convert_dtype="deprecated", but the public docstring never mentions that parameter or its 26.10 removal timeline, so generated API docs stay out of sync with the actual API. As per coding guidelines, "Missing or incorrect docstrings for public methods" and "Hyperparameters not documented" are HIGH issues.🤖 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/dask/manifold/umap.py` around lines 90 - 112, The public docstring for UMAP.transform is out of sync with the signature because it does not document convert_dtype="deprecated" or its planned removal in 26.10. Update the transform() docstring in the UMAP class to add a Parameters entry for convert_dtype, marking it deprecated and noting the removal timeline so the API docs match the actual method signature.Source: Coding guidelines
python/cuml/cuml/explainer/tree_shap.pyx (1)
158-166: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH:
TreeExplainer.__init__skips the deprecation warning whendata=None.Here
convert_dtypeonly flows throughcheck_arrayinside theif data is not Noneblock, soTreeExplainer(model=..., convert_dtype=True)is silently accepted and callers never get the 26.10 removal warning.🤖 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/explainer/tree_shap.pyx` around lines 158 - 166, TreeExplainer.__init__ only routes convert_dtype through check_array when data is provided, so the deprecation/removal warning is skipped for data=None. Update the TreeExplainer.__init__ path to always validate convert_dtype and emit the deprecation warning even when data is absent, using the existing check_array handling or an equivalent shared validation path so callers of TreeExplainer(model=..., convert_dtype=True) still see the 26.10 warning.
🤖 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/cluster/hdbscan/hdbscan.pyx`:
- Around line 1187-1192: The public helper docstrings for membership_vector and
approximate_predict are missing the deprecated convert_dtype kwarg, so update
both docstrings to explicitly document convert_dtype as deprecated and describe
its current behavior. Make sure the changes are applied in the definitions of
membership_vector and approximate_predict so the published API docs match the
runtime warning/removal behavior.
In `@python/cuml/cuml/explainer/tree_shap.pyx`:
- Line 158: The public TreeExplainer APIs are missing documentation for the
deprecated convert_dtype keyword, so update the docstrings for
TreeExplainer.__init__, TreeExplainer.shap_values, and
TreeExplainer.shap_interaction_values to explicitly mention convert_dtype, that
it is deprecated, and the intended removal timeline. Keep the parameter lists
consistent across these methods and ensure the public API docs reflect the
current signature exposed in tree_shap.pyx.
In `@python/cuml/cuml/linear_model/linear_regression.pyx`:
- Around line 313-320: The LinearRegression.fit signature currently makes
convert_dtype keyword-only, which breaks existing positional callers during the
deprecation window. Update fit in linear_regression.pyx to keep convert_dtype
accepting positional arguments while still emitting the deprecation warning, and
only enforce the keyword-only form after the removal period. Use the existing
LinearRegression.fit entry point and its convert_dtype handling to preserve
backward compatibility for callers like fit(X, y, sample_weight, False).
In `@python/cuml/cuml/tsa/arima.pyx`:
- Line 957: ARIMA.fit() is still using the deprecated dtype-conversion path by
default because it hardcodes convert_dtype=True before calling _loglike,
_loglike_grad, and unpack(). Update ARIMA.fit so its default matches the new
sentinel-based behavior and only opts into dtype conversion when the caller
explicitly requests it, then propagate that same sentinel value through the
_loglike/_loglike_grad/unpack call chain.
---
Outside diff comments:
In `@python/cuml/cuml/cluster/kmeans.pyx`:
- Around line 805-813: The `KMeans.fit_transform` path is not forwarding
`convert_dtype` into the fit step, so the kwarg is only applied in `transform`.
Update `fit_transform` to pass `convert_dtype` through to `self.fit(...)` as
well as `self.transform(...)`, matching the existing `fit`/`transform` behavior
and ensuring dtype handling is consistent across the full call.
In `@python/cuml/cuml/dask/manifold/umap.py`:
- Around line 90-112: The public docstring for UMAP.transform is out of sync
with the signature because it does not document convert_dtype="deprecated" or
its planned removal in 26.10. Update the transform() docstring in the UMAP class
to add a Parameters entry for convert_dtype, marking it deprecated and noting
the removal timeline so the API docs match the actual method signature.
In `@python/cuml/cuml/dask/neighbors/kneighbors_classifier.py`:
- Around line 280-299: The public method score in
kneighbors_classifier.KNeighborsClassifier still accepts convert_dtype but its
docstring does not mention that the parameter is deprecated. Update the score
docstring to document convert_dtype as a deprecated keyword, including a brief
deprecation/removal note in the Parameters section so users know how to migrate;
keep the change localized to the score method documentation.
In `@python/cuml/cuml/decomposition/incremental_pca.py`:
- Around line 205-224: Update the IncrementalPCA.fit docstring to document the
convert_dtype keyword and its deprecation status so the public API matches the
signature. In the fit method, add a Parameters entry for convert_dtype and note
that it is deprecated, aligning the wording with the existing transform()
documentation and making the deprecation/removal timeline explicit for callers.
- Around line 401-455: The dense path in IncrementalPCA.transform is dropping
the caller-provided convert_dtype value, so the behavior differs from the sparse
branch. Update transform so the explicit convert_dtype argument is forwarded
through the dense branch as well, using the existing super().transform path in a
way that preserves this parameter. Keep the sparse branch’s check_inputs flow
unchanged, and ensure the deprecation behavior for convert_dtype is exercised
consistently for both dense and sparse inputs.
In `@python/cuml/cuml/explainer/tree_shap.pyx`:
- Around line 158-166: TreeExplainer.__init__ only routes convert_dtype through
check_array when data is provided, so the deprecation/removal warning is skipped
for data=None. Update the TreeExplainer.__init__ path to always validate
convert_dtype and emit the deprecation warning even when data is absent, using
the existing check_array handling or an equivalent shared validation path so
callers of TreeExplainer(model=..., convert_dtype=True) still see the 26.10
warning.
In `@python/cuml/cuml/internals/validation.py`:
- Around line 1354-1370: `check_sample_weight` has input-dependent deprecation
behavior because `None` and scalar values return before `check_array(...)`, so
they skip the `convert_dtype` warning path. Update `check_sample_weight` so the
deprecation check runs for all inputs, including `None` and scalar
`sample_weight`, by moving or duplicating the `convert_dtype` handling ahead of
the early returns. Keep the existing validation logic in `check_sample_weight`
and `check_array`/`validate_sample_weight` flow consistent for array-like,
scalar, and `None` inputs.
- Around line 1131-1169: The `check_y(..., return_classes=True)` branch is
missing the `convert_dtype` deprecation warning that the non-`return_classes`
path already emits. Update `check_y` so the `return_classes` flow also routes
through the same deprecation handling around the internal `check_array(...)`
call, using the existing `convert_dtype` argument and warning logic consistently
in both branches.
In `@python/cuml/cuml/metrics/cluster/adjusted_rand_index.pyx`:
- Around line 22-37: Update the public docstring for adjusted_rand_score to
document the convert_dtype argument as deprecated, including the deprecation
notice and removal timeline. Make sure the Parameters section mentions
convert_dtype alongside labels_true and labels_pred, and reference the
adjusted_rand_score signature so the docstring matches the exposed API. Keep the
guidance explicit that this parameter is deprecated and should not be used in
new code.
In `@python/cuml/cuml/metrics/cluster/silhouette_score.pyx`:
- Around line 143-179: The public silhouette wrappers `cython_silhouette_score`
and `cython_silhouette_samples` accept `convert_dtype="deprecated"` but their
docstrings do not mention the deprecation or removal guidance. Update both
docstrings to document `convert_dtype` with a clear deprecation notice and
migration guidance so the public API matches the other metrics wrappers and
users see consistent guidance.
In `@python/cuml/cuml/metrics/pairwise_distances.pyx`:
- Around line 270-303: The early-return path in pairwise_distances for
metric="nan_euclidean" is dropping the caller’s convert_dtype value, which
bypasses the existing deprecation behavior. Update the pairwise_distances
dispatch so the nan_euclidean branch still forwards convert_dtype into the
downstream implementation instead of replacing it with the deprecation sentinel.
Make the change in the pairwise_distances function while preserving the current
warning/deprecation flow and backward-compatible behavior.
In `@python/cuml/cuml/metrics/pairwise_kernels.py`:
- Around line 181-187: `pairwise_kernels` is not forwarding the deprecated
`convert_dtype` argument into the shared input validation path, so the expected
deprecation warning never fires. Update `pairwise_kernels` to pass the
caller-provided `convert_dtype` through to `check_array()` (and any related
validation helper used before kernel computation), and keep the handling
consistent with the other metric wrappers so the FutureWarning is emitted
whenever this kwarg is used.
In `@python/cuml/cuml/neighbors/nearest_neighbors.pyx`:
- Around line 705-727: The sparse kneighbors path in nearest_neighbors.pyx is
still ignoring the caller’s explicit convert_dtype value, so update the
sparse-input handling in kneighbors() to pass convert_dtype through to
check_array just like the dense path. Make sure the same fix is applied wherever
the sparse branch is handled so explicit convert_dtype=True/False behavior is
preserved and the deprecation warning contract is triggered consistently.
In `@python/cuml/cuml/svm/svc.py`:
- Around line 417-437: The multiclass path in SVC.predict is bypassing the new
convert_dtype behavior, so the _multiclass branch should be updated to honor the
caller’s convert_dtype argument just like the binary path does. Adjust the
multiclass delegation in predict (and any shared multiclass helper it calls) so
legacy convert_dtype values still trigger the expected warning/handling and are
not silently ignored, while keeping decode_labels and _get_output_type
unchanged.
In `@python/cuml/tests/test_logistic_regression.py`:
- Around line 474-482: `test_logistic_predict_output_dtype` currently only
smoke-tests `cuLog.predict()` and does not validate the returned dtype/type.
Update this test to capture the result of
`clf.predict(X_test.astype(test_dtype))` and assert the expected prediction
dtype or array type explicitly, using the existing `dataset`, `test_dtype`, and
`cuLog` setup; if you intentionally want a smoke test, rename the test to
reflect that behavior instead.
🪄 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: ca7fe0b3-1b7c-47e5-b1a9-19e2cbfaee17
📒 Files selected for processing (74)
python/cuml/cuml/cluster/agglomerative.pyxpython/cuml/cuml/cluster/dbscan.pyxpython/cuml/cuml/cluster/hdbscan/hdbscan.pyxpython/cuml/cuml/cluster/kmeans.pyxpython/cuml/cuml/common/doc_utils.pypython/cuml/cuml/covariance/empirical_covariance.pypython/cuml/cuml/covariance/ledoit_wolf.pypython/cuml/cuml/dask/ensemble/randomforestclassifier.pypython/cuml/cuml/dask/ensemble/randomforestregressor.pypython/cuml/cuml/dask/manifold/umap.pypython/cuml/cuml/dask/neighbors/kneighbors_classifier.pypython/cuml/cuml/dask/neighbors/kneighbors_regressor.pypython/cuml/cuml/decomposition/incremental_pca.pypython/cuml/cuml/decomposition/pca.pyxpython/cuml/cuml/decomposition/tsvd.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/cuml/experimental/linear_model/lars.pyxpython/cuml/cuml/explainer/tree_shap.pyxpython/cuml/cuml/internals/validation.pypython/cuml/cuml/kernel_ridge/kernel_ridge.pypython/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/elastic_net.pypython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/logistic_regression.pypython/cuml/cuml/linear_model/mbsgd_classifier.pypython/cuml/cuml/linear_model/mbsgd_regressor.pypython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml/manifold/t_sne.pyxpython/cuml/cuml/manifold/umap/umap.pyxpython/cuml/cuml/metrics/cluster/adjusted_rand_index.pyxpython/cuml/cuml/metrics/cluster/silhouette_score.pyxpython/cuml/cuml/metrics/confusion_matrix.pypython/cuml/cuml/metrics/kl_divergence.pyxpython/cuml/cuml/metrics/pairwise_distances.pyxpython/cuml/cuml/metrics/pairwise_kernels.pypython/cuml/cuml/metrics/trustworthiness.pyxpython/cuml/cuml/naive_bayes/naive_bayes.pypython/cuml/cuml/neighbors/kernel_density.pyxpython/cuml/cuml/neighbors/kneighbors_classifier.pyxpython/cuml/cuml/neighbors/kneighbors_classifier_mg.pyxpython/cuml/cuml/neighbors/kneighbors_regressor.pyxpython/cuml/cuml/neighbors/kneighbors_regressor_mg.pyxpython/cuml/cuml/neighbors/nearest_neighbors.pyxpython/cuml/cuml/neighbors/nearest_neighbors_mg.pyxpython/cuml/cuml/random_projection/random_projection.pypython/cuml/cuml/solvers/cd.pyxpython/cuml/cuml/solvers/qn.pyxpython/cuml/cuml/solvers/sgd.pyxpython/cuml/cuml/svm/linear.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/linear_svr.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml/svm/svr.pypython/cuml/cuml/tsa/arima.pyxpython/cuml/cuml/tsa/auto_arima.pyxpython/cuml/cuml/tsa/seasonality.pypython/cuml/cuml/tsa/stationarity.pyxpython/cuml/tests/dask/test_dask_kneighbors_classifier.pypython/cuml/tests/dask/test_dask_kneighbors_regressor.pypython/cuml/tests/test_elastic_net.pypython/cuml/tests/test_kernel_density.pypython/cuml/tests/test_linear_regression.pypython/cuml/tests/test_logistic_regression.pypython/cuml/tests/test_metrics.pypython/cuml/tests/test_nearest_neighbors.pypython/cuml/tests/test_random_forest.pypython/cuml/tests/test_solver_attributes.pypython/cuml/tests/test_svm.pypython/cuml/tests/test_trustworthiness.pypython/cuml/tests/test_tsne.pypython/cuml/tests/test_umap.pypython/cuml/tests/test_validation.py
💤 Files with no reviewable changes (2)
- python/cuml/tests/test_nearest_neighbors.py
- python/cuml/tests/test_trustworthiness.py
97c3aed to
cc32c00
Compare
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/internals/validation.py (1)
1131-1169: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
convert_dtypeis silently dropped in the classification (return_classes) branch.When
return_classes is not False,check_ynever forwardsconvert_dtypeto the internalcheck_arraycall (line 1138), so an explicitconvert_dtype=True/Falsepassed by a classifier'sfit()never triggers the deprecationFutureWarninghere, unlike the regression branch (line 1170-1179). This makes the deprecation signal inconsistent depending on estimator type, andtest_convert_dtype_deprecateddoesn't cover this path.♻️ Proposed fix
if not isinstance(y, (cudf.DataFrame, cudf.Series)): y = check_array( y, + convert_dtype=convert_dtype, mem_type=None, ensure_2d=False, ensure_min_samples=0, ensure_all_finite=False, input_name="y", )🤖 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/internals/validation.py` around lines 1131 - 1169, The classification path in check_y currently ignores convert_dtype when return_classes is not False, so the deprecation warning path is skipped for classifier fits. Update the check_array call inside the return_classes branch to forward convert_dtype the same way the regression branch does, and keep the behavior consistent across both paths in check_y so the FutureWarning is emitted regardless of estimator type.
🧹 Nitpick comments (1)
python/cuml/cuml/internals/validation.py (1)
634-641: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
stacklevelto the deprecation warning.Without
stacklevel, theFutureWarningpoints at this internal frame incheck_arrayrather than the caller's code, making it harder for users to locate whereconvert_dtypeis still being passed.♻️ Proposed fix
if convert_dtype != "deprecated": warnings.warn( "`convert_dtype` was deprecated in version 26.08 and will be " "removed in version 26.10. cuML only copies input arrays when " "necessary (e.g. to unify dtypes), there is no reason to " "provide this keyword going forward.", FutureWarning, + stacklevel=2, )🤖 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/internals/validation.py` around lines 634 - 641, Add a stacklevel argument to the FutureWarning emitted in check_array’s convert_dtype deprecation branch so the warning points to the caller instead of the internal validation frame. Update the warnings.warn call in validation.py where convert_dtype != "deprecated" to include an appropriate stacklevel value, preserving the existing message and FutureWarning category.Source: Linters/SAST tools
🤖 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/internals/validation.py`:
- Around line 1131-1169: The classification path in check_y currently ignores
convert_dtype when return_classes is not False, so the deprecation warning path
is skipped for classifier fits. Update the check_array call inside the
return_classes branch to forward convert_dtype the same way the regression
branch does, and keep the behavior consistent across both paths in check_y so
the FutureWarning is emitted regardless of estimator type.
---
Nitpick comments:
In `@python/cuml/cuml/internals/validation.py`:
- Around line 634-641: Add a stacklevel argument to the FutureWarning emitted in
check_array’s convert_dtype deprecation branch so the warning points to the
caller instead of the internal validation frame. Update the warnings.warn call
in validation.py where convert_dtype != "deprecated" to include an appropriate
stacklevel value, preserving the existing message and FutureWarning category.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1f2471f8-3a05-4d9f-92a9-ba5d3d6cea93
📒 Files selected for processing (74)
python/cuml/cuml/cluster/agglomerative.pyxpython/cuml/cuml/cluster/dbscan.pyxpython/cuml/cuml/cluster/hdbscan/hdbscan.pyxpython/cuml/cuml/cluster/kmeans.pyxpython/cuml/cuml/common/doc_utils.pypython/cuml/cuml/covariance/empirical_covariance.pypython/cuml/cuml/covariance/ledoit_wolf.pypython/cuml/cuml/dask/ensemble/randomforestclassifier.pypython/cuml/cuml/dask/ensemble/randomforestregressor.pypython/cuml/cuml/dask/manifold/umap.pypython/cuml/cuml/dask/neighbors/kneighbors_classifier.pypython/cuml/cuml/dask/neighbors/kneighbors_regressor.pypython/cuml/cuml/decomposition/incremental_pca.pypython/cuml/cuml/decomposition/pca.pyxpython/cuml/cuml/decomposition/tsvd.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/cuml/experimental/linear_model/lars.pyxpython/cuml/cuml/explainer/tree_shap.pyxpython/cuml/cuml/internals/validation.pypython/cuml/cuml/kernel_ridge/kernel_ridge.pypython/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/elastic_net.pypython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/logistic_regression.pypython/cuml/cuml/linear_model/mbsgd_classifier.pypython/cuml/cuml/linear_model/mbsgd_regressor.pypython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml/manifold/t_sne.pyxpython/cuml/cuml/manifold/umap/umap.pyxpython/cuml/cuml/metrics/cluster/adjusted_rand_index.pyxpython/cuml/cuml/metrics/cluster/silhouette_score.pyxpython/cuml/cuml/metrics/confusion_matrix.pypython/cuml/cuml/metrics/kl_divergence.pyxpython/cuml/cuml/metrics/pairwise_distances.pyxpython/cuml/cuml/metrics/pairwise_kernels.pypython/cuml/cuml/metrics/trustworthiness.pyxpython/cuml/cuml/naive_bayes/naive_bayes.pypython/cuml/cuml/neighbors/kernel_density.pyxpython/cuml/cuml/neighbors/kneighbors_classifier.pyxpython/cuml/cuml/neighbors/kneighbors_classifier_mg.pyxpython/cuml/cuml/neighbors/kneighbors_regressor.pyxpython/cuml/cuml/neighbors/kneighbors_regressor_mg.pyxpython/cuml/cuml/neighbors/nearest_neighbors.pyxpython/cuml/cuml/neighbors/nearest_neighbors_mg.pyxpython/cuml/cuml/random_projection/random_projection.pypython/cuml/cuml/solvers/cd.pyxpython/cuml/cuml/solvers/qn.pyxpython/cuml/cuml/solvers/sgd.pyxpython/cuml/cuml/svm/linear.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/linear_svr.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml/svm/svr.pypython/cuml/cuml/tsa/arima.pyxpython/cuml/cuml/tsa/auto_arima.pyxpython/cuml/cuml/tsa/seasonality.pypython/cuml/cuml/tsa/stationarity.pyxpython/cuml/tests/dask/test_dask_kneighbors_classifier.pypython/cuml/tests/dask/test_dask_kneighbors_regressor.pypython/cuml/tests/test_elastic_net.pypython/cuml/tests/test_kernel_density.pypython/cuml/tests/test_linear_regression.pypython/cuml/tests/test_logistic_regression.pypython/cuml/tests/test_metrics.pypython/cuml/tests/test_nearest_neighbors.pypython/cuml/tests/test_random_forest.pypython/cuml/tests/test_solver_attributes.pypython/cuml/tests/test_svm.pypython/cuml/tests/test_trustworthiness.pypython/cuml/tests/test_tsne.pypython/cuml/tests/test_umap.pypython/cuml/tests/test_validation.py
💤 Files with no reviewable changes (2)
- python/cuml/tests/test_nearest_neighbors.py
- python/cuml/tests/test_trustworthiness.py
✅ Files skipped from review due to trivial changes (10)
- python/cuml/cuml/neighbors/nearest_neighbors_mg.pyx
- python/cuml/tests/test_linear_regression.py
- python/cuml/cuml/svm/linear.pyx
- python/cuml/cuml/kernel_ridge/kernel_ridge.py
- python/cuml/cuml/tsa/auto_arima.pyx
- python/cuml/cuml/neighbors/kneighbors_classifier_mg.pyx
- python/cuml/tests/dask/test_dask_kneighbors_classifier.py
- python/cuml/cuml/random_projection/random_projection.py
- python/cuml/tests/test_solver_attributes.py
- python/cuml/tests/dask/test_dask_kneighbors_regressor.py
🚧 Files skipped from review as they are similar to previous changes (61)
- python/cuml/cuml/neighbors/kneighbors_regressor_mg.pyx
- python/cuml/cuml/tsa/stationarity.pyx
- python/cuml/cuml/linear_model/ridge.pyx
- python/cuml/cuml/tsa/seasonality.py
- python/cuml/cuml/svm/svm_base.pyx
- python/cuml/tests/test_elastic_net.py
- python/cuml/cuml/svm/linear_svc.py
- python/cuml/cuml/decomposition/tsvd.pyx
- python/cuml/cuml/cluster/agglomerative.pyx
- python/cuml/cuml/metrics/cluster/adjusted_rand_index.pyx
- python/cuml/cuml/linear_model/linear_regression.pyx
- python/cuml/cuml/experimental/linear_model/lars.pyx
- python/cuml/cuml/explainer/tree_shap.pyx
- python/cuml/cuml/metrics/confusion_matrix.py
- python/cuml/cuml/linear_model/mbsgd_classifier.py
- python/cuml/cuml/metrics/cluster/silhouette_score.pyx
- python/cuml/cuml/covariance/empirical_covariance.py
- python/cuml/cuml/dask/manifold/umap.py
- python/cuml/cuml/svm/svr.py
- python/cuml/cuml/linear_model/logistic_regression.py
- python/cuml/cuml/neighbors/kneighbors_regressor.pyx
- python/cuml/cuml/dask/ensemble/randomforestregressor.py
- python/cuml/tests/test_random_forest.py
- python/cuml/cuml/decomposition/pca.pyx
- python/cuml/cuml/solvers/sgd.pyx
- python/cuml/cuml/metrics/kl_divergence.pyx
- python/cuml/cuml/cluster/hdbscan/hdbscan.pyx
- python/cuml/cuml/solvers/cd.pyx
- python/cuml/cuml/dask/ensemble/randomforestclassifier.py
- python/cuml/cuml/linear_model/elastic_net.py
- python/cuml/cuml/linear_model/base.py
- python/cuml/tests/test_svm.py
- python/cuml/cuml/dask/neighbors/kneighbors_classifier.py
- python/cuml/cuml/linear_model/mbsgd_regressor.py
- python/cuml/cuml/svm/svc.py
- python/cuml/cuml/neighbors/kneighbors_classifier.pyx
- python/cuml/cuml/metrics/pairwise_kernels.py
- python/cuml/cuml/svm/linear_svr.py
- python/cuml/cuml/covariance/ledoit_wolf.py
- python/cuml/cuml/ensemble/randomforestregressor.py
- python/cuml/cuml/neighbors/kernel_density.pyx
- python/cuml/cuml/solvers/qn.pyx
- python/cuml/tests/test_tsne.py
- python/cuml/cuml/manifold/t_sne.pyx
- python/cuml/cuml/metrics/trustworthiness.pyx
- python/cuml/cuml/ensemble/randomforestclassifier.py
- python/cuml/cuml/decomposition/incremental_pca.py
- python/cuml/cuml/cluster/dbscan.pyx
- python/cuml/tests/test_kernel_density.py
- python/cuml/cuml/metrics/pairwise_distances.pyx
- python/cuml/tests/test_validation.py
- python/cuml/cuml/dask/neighbors/kneighbors_regressor.py
- python/cuml/cuml/tsa/arima.pyx
- python/cuml/tests/test_logistic_regression.py
- python/cuml/cuml/cluster/kmeans.pyx
- python/cuml/cuml/common/doc_utils.py
- python/cuml/cuml/naive_bayes/naive_bayes.py
- python/cuml/cuml/neighbors/nearest_neighbors.pyx
- python/cuml/cuml/manifold/umap/umap.pyx
- python/cuml/tests/test_umap.py
- python/cuml/tests/test_metrics.py
betatim
left a comment
There was a problem hiding this comment.
There are a few cases where a function/method accepts convert_dtype but the argument doesn't get used in some branches/doesn't make its way to check_array. This means users don't get the deprecation warning, and in the future will get a (surprising) error.
The tricky part is, that we can't just thread this argument through (the obvious way to get a warning in all cases), because it can change what the code does for those who have been passing this currently ignored argument.
SVC.predictin the multiclass casecuml.metrics.pairwise_distances(..., metric="nan_euclidean")IncrementalPCA.transform()for dense inputs
|
We had some private discussion about the functions/methods that take |
|
/merge |
This deprecates the
convert_dtypekwarg everywhere incuml.See #7646 for more information. In short, we're removing it for the following reasons:
There is no replacement option, as the parameter either led to errors or did nothing if the inputs were of the required dtype. Users who were providing this parameter before should just stop using it.
Fixes #7646.