-
Notifications
You must be signed in to change notification settings - Fork 554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multiple CPU interop fixes for serialization and cloning #6223
Open
dantegd
wants to merge
18
commits into
rapidsai:branch-25.04
Choose a base branch
from
dantegd:fix-interop-fixes
base: branch-25.04
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
3d0760e
FIX fix cloning
dantegd ca66732
Merge branch 'branch-25.02' into fix-interop-fixes
dantegd 497d181
FIX changes from PR review to not use internal sklearn APIs and mate …
dantegd dce1539
Merge cuML branch-25.04
dantegd 0d797aa
Merge branch 'branch-25.04' into fix-interop-fixes
dantegd 0ef4895
ENH Keep list of original hyperparams that user passed
dantegd a00af9d
Merge branch 'fix-interop-fixes' of github.com:dantegd/cuml into fix-…
dantegd 4e70f4c
FIX remove unused imported function
dantegd de3e234
Check that get_params and cloning work
betatim f14a14b
Typo fix
betatim 5a02fd0
ENH multiple improvements by using the cpu_model as the reference tru…
dantegd e0cd0d5
FIX style fixes
dantegd d206bb8
Merge cuML branch-25.04
dantegd e70c3fb
Merge branch 'branch-25.04' into fix-interop-fixes
dantegd 3785c4e
DOC correct docstrings
dantegd fd17f09
Merge branch 'fix-interop-fixes' of github.com:dantegd/cuml into fix-…
dantegd e7a35a1
Move imports to the top
betatim 742404e
Fix style
betatim File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,8 @@ | |
|
||
import pytest | ||
import numpy as np | ||
import cupy as cp | ||
from sklearn import clone, cluster | ||
import cuml | ||
from sklearn.datasets import make_classification, make_regression, make_blobs | ||
from sklearn.linear_model import ( | ||
LinearRegression, | ||
|
@@ -173,6 +174,57 @@ def test_proxy_facade(): | |
assert original_value == proxy_value | ||
|
||
|
||
def test_proxy_clone(): | ||
# Test that cloning a proxy estimator preserves parameters, even those we | ||
# translate for the cuml class | ||
pca = PCA(n_components=42, svd_solver="arpack") | ||
pca_clone = clone(pca) | ||
|
||
assert pca.get_params() == pca_clone.get_params() | ||
|
||
|
||
def test_proxy_params(): | ||
# Test that parameters match between constructor and get_params() | ||
# Mix of default and non-default values | ||
pca = PCA( | ||
n_components=5, | ||
copy=False, | ||
# Pass in an argument and set it to its default value | ||
whiten=False, | ||
) | ||
|
||
params = pca.get_params() | ||
assert params["n_components"] == 5 | ||
assert params["copy"] is False | ||
assert params["whiten"] is False | ||
# A parameter we never touched, should be the default | ||
assert params["tol"] == 0.0 | ||
|
||
# Check that get_params doesn't return any unexpected parameters | ||
expected_params = set( | ||
[ | ||
"n_components", | ||
"copy", | ||
"whiten", | ||
"tol", | ||
"svd_solver", | ||
"n_oversamples", | ||
"random_state", | ||
"iterated_power", | ||
"power_iteration_normalizer", | ||
] | ||
) | ||
assert set(params.keys()) == expected_params | ||
|
||
|
||
def test_roundtrip(): | ||
|
||
km = cluster.KMeans(n_clusters=13) | ||
ckm = cuml.KMeans.from_sklearn(km) | ||
|
||
assert ckm.n_clusters == 13 | ||
|
||
|
||
def test_defaults_args_only_methods(): | ||
# Check that estimator methods that take no arguments work | ||
# These are slightly weird because basically everything else takes | ||
|
@@ -186,6 +238,8 @@ def test_defaults_args_only_methods(): | |
|
||
|
||
def test_kernel_ridge(): | ||
import cupy as cp | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why move this here? Maybe we should leave a comment for people from the future to explain why it can't be imported at the top of the file (or move it back if this was just for debugging) |
||
|
||
rng = np.random.RandomState(42) | ||
|
||
X = 5 * rng.rand(10000, 1) | ||
|
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lucky number 13 :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should merge this PR. It improves things and fixes several things.
We can keep improving the
from_
/as_sklearn
round tripping. I think the test from https://github.com/rapidsai/cuml/pull/6342/files#r1963552769 still doesn't pass (even if you exclude the raft handle). But lets look at that in a new PR