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
TST activate common tests for TSNE #25374
base: main
Are you sure you want to change the base?
Conversation
TSNE is not a usual transformer unfortunately:
shall we use the dedicated estimator tag to xfail those specific checks explicitly? |
@@ -537,7 +537,7 @@ def trustworthiness(X, X_embedded, *, n_neighbors=5, metric="euclidean"): | |||
return t | |||
|
|||
|
|||
class TSNE(BaseEstimator): | |||
class TSNE(ClassNamePrefixFeaturesOutMixin, TransformerMixin, BaseEstimator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it weird to have a sub-class of TransformerMixin
without a transform
method. But maybe it's better than what we do currently...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the Mixin
only defines fit_transform
and we override it, I find it fine to inherit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem we are trying to solve by inheriting is having TSNE
selected as a transformer by all_estimators
. Is there another way of doing that, other than having to inherit from TransformerMixin
?
It feels weird to inherit, which for me is saying "I am a transformer with all the things a transformer can do!" and then not have a transform
method. But the mixin doesn't define it either. So is having a transform
method not part of being "a real transformer"?
On a pragmatic level, using the mixin is a nice way to get into the list of all_estimators
and maybe mixins aren't like real "is a" style inheritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I am a transformer with all the things a transformer can do!"
Looking at the documentation, it seems that the original spirit is indeed to have at least fit
and transform
and fit_transform
is just a convenience.
On a pragmatic level, using the mixin is a nice way to get into the list of all_estimators and maybe mixins aren't like real "is a" style inheritance?
We could always replace by duck-typing: if an estimator implements fit
+transform
or/and fit_transform
, then it should pass the test of a transformer.
Indeed, some of our checks are ducktyping while the helper to list the estimators is checking for the mixins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the annoying part with duck-typing is that we already need to have an instance while we are playing with classes at this stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand what you mean. To find out if a method is implemented you could do something like "fit" in dir(Estimator)
no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed we could. I just had in my mind a piece of the parameter validation framework HasMethods
that does this job already for instances.
linked to #25365
This PR intends to activate the common test for
TSNE
. While it does not implementtransform
, it should still pass the common tests forfit_transform
.