10000 Allow overwriting tags by rewriting the mechanism to use inheritance by amueller · Pull Request #14635 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Allow overwriting tags by rewriting the mechanism to use inheritance #14635

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
wants to merge 4 commits into from
Closed

Allow overwriting tags by rewriting the mechanism to use inheritance #14635

wants to merge 4 commits into from

Conversation

amueller
Copy link
Member

Fixes #14044.

This should work but might be considered slightly hacky.
It avoids reordering mixins and base estimator by creating a common base class.
I would argue it would be nicer to not use mixins and use inheritance instead and make the mixins ABCs.

This is semi-backward-incompatible for third-party authors. If they had a hierarchy of classes that added (different) tags in each class using _more_tags, this will now break as _more_tags is only called on self and not on the full MRO.
If they only have one class which defines _more_tags I think this should work.

This rewrite basically gets rid of _more_tags which could be deprecated.

@amueller amueller mentioned this pull request Aug 12, 2019
Closed
@amueller amueller mentioned this pull request Aug 12, 2019
Merged
@jnothman
Copy link
Member

So the problem this solves is basically that it's currently hard to know what changing _more_tags will do given some inheritance hierarchy. super() semantics are clearer and more standard. Is that right?

@NicolasHug
Copy link
Member
NicolasHug commented Aug 13, 2019 •
edited
Loading

@jnothman I think the real problem that it solves is that with the current _more_tags() logic, you can only override the default of a tag once in the hierarchy.

@amueller remind me why we need the TagsBase class here? BaseEstimator isn't enough?

@amueller
Copy link
Member Author

@jnothman The explanation by @NicolasHug is pretty on point. Previously we didn't allow overwriting tags in the hierarchy, or rather we allowed changing them exactly once. Which is not flexible enough for many cases.
This PR uses the super mechanism of resolution across the class hierarchy, which I would argue is the right thing to do. It does mean that we depend on the inheritance order.

@NicolasHug as mentioned, it might be possible to do without it. I'll send a different PR that will probably work by changing the inheritance order instead of introducing a base class.

@amueller amueller mentioned this pull request Aug 13, 2019
Closed
@amueller
Copy link
Member Author
amueller commented Sep 5, 2019

closing for now, I prefer #14644

@amueller amueller closed this Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Estimator tag overwriting and update in _get_tags
3 participants
@amueller @jnothman @NicolasHug
0