Skip to content

FIX proper inheritance for SGDOneClassSVM #30227

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

Merged

Conversation

glemaitre
Copy link
Member

Fixes main

This pull request addresses an issue with the SGDOneClassSVM class in the sklearn.linear_model module, ensuring it correctly inherits from OutlierMixin and sets the appropriate tags. The most important changes include modifying the class inheritance, updating the documentation, and adding a new test to verify the correct estimator type.

@glemaitre
Copy link
Member Author

glemaitre commented Nov 5, 2024

Copy link

github-actions bot commented Nov 5, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 37b9f0a. Link to the linter CI: here

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

Side note: In the past, I wanted to add a mixin order check to the common test, but I was unsure around running it on non scikit-learn estimators.

@glemaitre
Copy link
Member Author

Side note: In the past, I wanted to add a mixin order check to the common test, but I was unsure around running it on non scikit-learn estimators.

When adding the non-regression test, I had exactly this thought :). If we would have a fine grade test suite, we could have a categories for the estimators inheriting from our mixin and base estimator. Basically, even third-party will need this order of inheritance.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

In terms of a common test, I think it should be in the API level anyway. At this point, if third parties don't inherit from our mixins, they really can't be compatible with us.

@adrinjalali adrinjalali enabled auto-merge (squash) November 5, 2024 19:34
@adrinjalali adrinjalali merged commit 102663d into scikit-learn:main Nov 5, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants