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.

Sign up for GitHub

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

Jump to bottom

[BUG] Not all estimators match scikit-learn signature conventions #6255

Closed
jcrist opened this issue Jan 23, 2025 · 0 comments · Fixed by #6260
Closed

[BUG] Not all estimators match scikit-learn signature conventions #6255

jcrist opened this issue Jan 23, 2025 · 0 comments · Fixed by #6260
Assignees
@jcrist
Labels
? - Needs Triage Need team to review and classify bug Something isn't working

Comments

@jcrist
Copy link
Member

jcrist commented Jan 23, 2025

Not all estimators in cuml match the expected scikit-learn signatures for fit/fit_transform. In particular, many estimators are missing a y parameter to fit/fit_transform in cases where y would be ignored (but scikit-learn still requires it to be there). This can lead to issues when trying to switch between scikit-learn and cuml estimators.

We should:

  • Add a test that ensures the standard methods (fit/fit_transform/transform/predict) match the expected standard signatures
  • Fix cases where they don't
@jcrist jcrist added ? - Needs Triage Need team to review and classify bug Something isn't working labels Jan 23, 2025
@jcrist jcrist self-assigned this Jan 23, 2025
@jcrist jcrist mentioned this issue Jan 24, 2025
Merged
rapids-bot bot pushed a commit that referenced this issue Jan 27, 2025
@jcrist
Ensure all method signatures are sklearn compatible (#6260)
3a32228 
`sklearn` requires `fit`/`fit_transform`/... always take a `y` parameter, even if it's ignored. This adds a test to ensure our signatures match this rule, and fixes any cases where they didn't. This makes it easier to include `cuml` estimators within sklearn pipelines.

Fixes #6255.

Authors:
  - Jim Crist-Harif (https://github.com/jcrist)
  - Bradley Dice (https://github.com/bdice)
  - Tim Head (https://github.com/betatim)

Approvers:
  - William Hicks (https://github.com/wphicks)
  - Tim Head (https://github.com/betatim)

URL: #6260
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jcrist jcrist

Labels
? - Needs Triage Need team to review and classify bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Ensure all method signatures are sklearn compatible jcrist/cuml
1 participant
@jcrist