Skip to content

[MRG+1] TST: estimator_checks: skip transform test when not implemented #6510

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

Closed

Conversation

jakevdp
Copy link
Member

@jakevdp jakevdp commented Mar 8, 2016

Estimator checks fail when a transformer only implements fit_transform.

@jakevdp jakevdp force-pushed the estimator-check-transform branch from 1cf68bc to f09a1bd Compare March 8, 2016 22:20
@amueller
Copy link
Member

amueller commented Mar 8, 2016

we have models that don't have transform, like TSNE. I think they don't inherit from TransformerMixin - if you can't transform, you're not a transformer.

@jakevdp
Copy link
Member Author

jakevdp commented Mar 8, 2016

if you can't transform, you're not a transformer.

In that case, should we remove similar checks that are already in there? e.g. https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py#L578

@amueller
Copy link
Member

amueller commented Mar 8, 2016

hm ok that is indeed inconsistent. I'm just a bit confused that we are not running into any troubles in master.

@jakevdp
Copy link
Member Author

jakevdp commented Mar 8, 2016

I believe it's because we're explicitly leaving out by name the transformers that don't have a transform method: https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py#L180

This, of course, doesn't help third-party estimators that we'd like to exclude as well.

@jnothman
Copy link
Member

jnothman commented Mar 8, 2016

All the exceptions by name have to go!

On 9 March 2016 at 10:32, Jake Vanderplas notifications@github.com wrote:

I believe it's because we're explicitly leaving out by name the
transformers that don't have a transform method:
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py#L180

This, of course, doesn't help third-party estimators that we'd like to
exclude as well.


Reply to this email directly or view it on GitHub
#6510 (comment)
.

@amueller amueller added this to the 0.19 milestone Oct 8, 2016
@amueller amueller mentioned this pull request Jun 12, 2017
4 tasks
@jnothman
Copy link
Member

This solution LGTM, but I would like a comment that "may have fit_transform but not transform"

@jnothman jnothman changed the title TST: estimator_checks: skip transform test when not implemented [MRG+1] TST: estimator_checks: skip transform test when not implemented Jun 14, 2017
@amueller
Copy link
Member

I still think that a transformer needs a transform method. Unfortunately it's unclear to me now what exception @jakevdp pointed to because line-numbers move :-/
All exceptions by name are gone in #8022

@jnothman
Copy link
Member

jnothman commented Jun 19, 2017 via email

@GKjohns GKjohns mentioned this pull request Dec 6, 2017
@amueller
Copy link
Member

@jnothman yes TSNE is not a transformer ;)

@rth
Copy link
Member

rth commented Jun 14, 2019

I still think that a transformer needs a transform method

Closing this issue from 2016 due to reviewer disagreement about this being the right solution. Currently, a better way of achieving this would probably involve estimator tags.

@rth rth closed this Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants