8000 Have a common test in check_estimator checking for the right order of mixin inheritance · Issue #30228 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Have a common test in check_estimator checking for the right order of mixin inheritance #30228

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
Closed
adrinjalali opened this issue Nov 5, 2024 · 2 comments · Fixed by #30234
Closed

Have a common test in check_estimator checking for the right order of mixin inheritance #30228

adrinjalali opened this issue Nov 5, 2024 · 2 comments · Fixed by #30234
Labels
Developer API Third party developer API related

Comments

@adrinjalali
Copy link
Member

xref: #30227 (review)

We should make sure it goes the right way, so that tags are set correctly, and to avoid other potential issues.
@github-actions github-actions bot added the Needs Triage Issue requires triage label Nov 5, 2024
@adrinjalali adrinjalali added Developer API Third party developer API related and removed Needs Triage Issue requires triage labels Nov 5, 2024
@adam2392
Copy link
Member
adam2392 commented Nov 7, 2024

I guess the main thought is that tags, and certain class attributes will get overwritten depending on the sequence of the inheritance?

In general though, our Mixin classes are relatively "mutually exclusive" right?

This would only be an issue if for some reason, someone inherited first from Mixin, and then BaseEstimator, or some other Base*, where some attributes/tags get overwritten?

@adrinjalali
Copy link
Member Author

Yeah the main concern is BaseEstimator needing to be the last one of our mixins in MRO. I'll write up a test, that should make it clear, and if we need to add more constraints, we can do that

@adrinjalali adrinjalali mentioned this issue Nov 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Developer API Third party developer API related
Projects
Developer API and third party library support
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

TST check inheritance order in common tests adrinjalali/scikit-learn
2 participants
@adrinjalali @adam2392
0