Skip to content

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

Closed
amueller wants to merge 4 commits intoscikit-learn:masterfrom
amueller:redo_class_hierachy
Closed

Allow overwriting tags by rewriting the mechanism to use inheritance#14635
amueller wants to merge 4 commits intoscikit-learn:masterfrom
amueller:redo_class_hierachy

Conversation

@amueller
Copy link
Copy Markdown
Member

@amueller amueller commented Aug 12, 2019

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
Copy Markdown
Member

jnothman commented Aug 13, 2019

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
Copy Markdown
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
Copy Markdown
Member Author

amueller commented Aug 13, 2019

@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
Copy Markdown
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