fix mixin inheritance order, allow overwriting tags#14884
fix mixin inheritance order, allow overwriting tags#14884rth merged 7 commits intoscikit-learn:masterfrom
Conversation
rth
left a comment
There was a problem hiding this comment.
I have not checked that changes include all classes, but the included changes LGTM.
I would really prefer using super but this is a mostly backward-compatible change and seems less controversial.
OK, but I think in either case these changes with mixin reordering would be necessary so it's good to have this no?
cc @NicolasHug in case you have bandwidth for a second quick review :)
|
Yes these changes here are are prerequisite so I'm happy to merge them. |
|
If we want to simulate super we should go down the MRO, not up. from sklearn.base import BaseEstimator
class A(BaseEstimator):
def _more_tags(self):
return {'allow_nan': True,
'multioutput': False}
class B(A):
def _more_tags(self):
return {'multioutput': True}
class C(B):
def _more_tags(self):
return {'allow_nan': False}
class D(C):
# No _more_tags(), yet self._more_tags() exists and it's C's method
pass
# "correctly" resolves to C's allow_nan only because C has _more_tags().
print(D()._get_tags()['allow_nan']) # False, OK
# resolves to A's multioutput instead of B's since C's _more_tags does not
# specifies multioutput, and because we're going up the mro instead of down the
# mro.
print(D()._get_tags()['multioutput']) # False, KO |
|
I need to make sure I have the tests from the other PR, will fix this later. |
sklearn/tests/test_base.py
Outdated
| diamond_tag_est = DiamondOverwriteTag() | ||
| with pytest.raises(TypeError, match="Inconsistent values for tag"): | ||
| diamond_tag_est._get_tags() | ||
| assert diamond_tag_est._get_tags() |
There was a problem hiding this comment.
lol this is missing an ['allow_nan']
|
I'm confused why the current tests pass ... |
|
tests were passing because the logic was to complicated and only wrong in complex cases. |
|
Thanks! |
|
In a follow up, we should document how the order of the inheritance matters now when using tags. |
* see scikit-learn/scikit-learn#14884 * see https://twitter.com/dabeaz/status/809084586487664641 * BaseEnsemble already inherits from MetaEstimatorMixin
Incremental change from #14644, keeping the old logic manually looping over the MRO.
I would really prefer using
superbut this is a mostly backward-compatible change and seems less controversial.