Prepare for n_init=auto in KMeans #6142
Conversation
|
I'd recommend we switch in 25.04 to get compatibility with the scikit-learn change as soon as possible. |
|
Works for me. There is a PR for a forward merge to 25.02, which makes me think that there won't be a 25.04? Is that right? 25.05 in that case? |
| sklearn_args = inspect.signature(self._cpu_model_class) | ||
| sklearn_args = sklearn_args.bind(*args, **kwargs) | ||
| sklearn_args.apply_defaults() |
There was a problem hiding this comment.
This means we use the constructor arguments and their default values of the scikit-learn class, combine them with what the user passed in and then feed it to the hyperparameter translator. This makes a difference for cases where the default values in scikit-learn and cuml are different and the user does not explicitly pass that argument.
# The scikit-learn class
class SkTimMeans:
def __init__(self, foo='bar'):
...
# The cuml class
class CuTimMeans:
def __init__(self, foo='baz'):
...
# User code
est = SkTimMeans()foo should be set to 'bar' in est because that is the default value of the scikit-learn class.
It also fixes the many deprecation warnings in the accelerator tests we were getting for KMeans due to the main change of this PR.
There was a problem hiding this comment.
This change is breaking quite a few tests and is orthogonal to the change in n_init, it'd be better to revert it and work on it on a follow up PR, since having KMeans init auto is the objective on this PR
There was a problem hiding this comment.
It wasn't totally unrelated. It solves the problem that without this the user would get a warning about the deprecation, even though for a scikit-learn user the default is "auto" already (based on the scikit-learn docs).
Switched to using the cuml class'es defaults to see if this creates less breakage.
If we don't use the signature of the constructor then we need to build a way for the hyper-parameter translator to translate arguments that the user didn't pass (I think this is the fundamental issue)
There was a problem hiding this comment.
With this approach we get failures like this:
sklearn.utils._param_validation.InvalidParameterError: The 'init' parameter of KMeans must be a str among {'random', 'k-means++'}, a callable or an array-like. Got 'scalable-k-means++' instead.
because the cuml default values are different from the scikit-learn ones :-/
| sklearn_args = inspect.signature(self._cpu_model_class) | ||
| sklearn_args = sklearn_args.bind(*args, **kwargs) | ||
| sklearn_args.apply_defaults() |
There was a problem hiding this comment.
This change is breaking quite a few tests and is orthogonal to the change in n_init, it'd be better to revert it and work on it on a follow up PR, since having KMeans init auto is the objective on this PR
This adds a warning for the upcoming switch from n_init=1 to 'auto'. This adds the possibility to use 'auto', which helps being compatible with sickit-learn.
This way the hyper-parameter translator gets to see all arguments and we avoid deprecation warnings due to mismatches between the sklearn and cuml defaults
Co-authored-by: Dante Gama Dessavre <[email protected]>
betatim
force-pushed
the
kmeans-auto-2
branch
from
6866248 to
7eaf3a1
Compare
betatim
added
improvement
|
Fixed up the last failing test, hopefully. |
|
The tests are failing with a |
|
There are two failures here: The one in We can silence the warning with a decorator on |
betatim
dismissed
dantegd’s stale review
I think the review comments are outdated now
|
/merge |
PRs being backported: - [x] #6234 - [x] #6306 - [x] #6320 - [x] #6319 - [x] #6327 - [x] #6333 - [x] #6142 - [x] #6223 - [x] #6235 - [x] #6317 - [x] #6331 - [x] #6326 - [x] #6332 - [x] #6347 - [x] #6348 - [x] #6337 - [x] #6355 - [x] #6354 - [x] #6322 - [x] #6353 - [x] #6359 - [x] #6364 - [x] #6363 - [x] [FIL BATCH_TREE_REORG fix for SM90, 100 and 120](a3e419a) --------- Co-authored-by: William Hicks <[email protected]>
Relevant PRs: [cuml 6235](rapidsai/cuml#6235), [cuml 6142](rapidsai/cuml#6142), for sklearn compatibility. - n_init defaults to "warn" as a future warning for the user (still ends up defaulting to 1), since the default will change to "auto" in 25.04 to match sklearn. - LR output shape changed from (n_classes, n_rows) to (n_rows, n_classes) to match sklearn. Resolves #860 --------- Signed-off-by: Rishi Chandra <[email protected]>
|
@betatim Can you create a follow-up to remove this now obsolete code segment relating to this PR? cuml/python/cuml/cuml/tests/test_sklearn_import_export.py Lines 106 to 110 in 3a8ea8c |
Follow up to #6142 (comment) This cleans up some leftovers from the deprecation cycle for `KMeans`'s `init="auto"` Authors: - Tim Head (https://github.com/betatim) - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Jim Crist-Harif (https://github.com/jcrist) URL: #6445
This adds a warning for the upcoming switch from n_init=1 to 'auto'. This adds the possibility to use 'auto', which helps being compatible with sickit-learn.
In which version should we switch to the new default?