Skip to content
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

Use estimator tags to improve sparse error handling #6151

Merged
merged 21 commits into from
Dec 13, 2024

Conversation

dantegd
Copy link
Member

@dantegd dantegd commented Dec 2, 2024

No description provided.

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Dec 2, 2024
Copy link

copy-pr-bot bot commented Dec 11, 2024

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@dantegd dantegd changed the base branch from branch-24.12 to branch-25.02 December 11, 2024 03:18
@dantegd dantegd added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Dec 11, 2024
@dantegd dantegd marked this pull request as ready for review December 11, 2024 03:19
@dantegd dantegd requested a review from a team as a code owner December 11, 2024 03:19
@dantegd dantegd requested review from csadorf and wphicks December 11, 2024 03:19
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One stray print statement and one question on testing.

@pytest.mark.parametrize("estimator_name", list(estimators.keys()))
def test_sparse_support(estimator_name):
X_sparse = csr_matrix([[0, 1], [1, 0]])
print(X_sparse.shape[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print(X_sparse.shape[0])



@pytest.mark.parametrize("estimator_name", list(estimators.keys()))
def test_sparse_support(estimator_name):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we feel about adding a test that confirms that an estimator without sparse support raises the correct exception on sparse input?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added them in a separate file to test vanilla cuML exceptions

@@ -122,13 +122,19 @@ def custom_weights(distances):
assert acc > 0.7, "Accuracy should be reasonable with custom weights"


@pytest.mark.xfail(
reason="cuML and sklearn don't have matching exceptions yet"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an issue logged for this?

@dantegd
Copy link
Member Author

dantegd commented Dec 13, 2024

/merge

@rapids-bot rapids-bot bot merged commit 7211507 into rapidsai:branch-25.02 Dec 13, 2024
61 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants