Skip to content

Conversation

@finnoshea
Copy link
Contributor

to emphasze that it performs a fit on X before returning the predictions on X

Reference Issues/PRs

What does this implement/fix? Explain your changes.

The docstring for OutlierMixin.fit_predict in sklearn/base.py only says that it makes predictions on X and not that it also performs a fit first. I have modified the docstring to show that it does perform the fit first. Somewhere I got the idea that fit_predict was for predictions on the data that was fit, but it is obviously not.

Any other comments?

…ze that it performs a fit on X before returning the predictions on X
Copy link
Contributor

@albertcthomas albertcthomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification.

@albertcthomas
Copy link
Contributor

Somewhere I got the idea that fit_predict was for predictions on the data that was fit

Note that you always have fit(X).predict(X) = fit_predict(X) when both predict and fit_predict exist.

@nixphix
Copy link
Contributor

nixphix commented Jan 11, 2019

@finnoshea have you created an issue for this PR? It's always good idea to open an issue before submitting a PR.

@finnoshea
Copy link
Contributor Author

Note that you always have fit(X).predict(X) = fit_predict(X) when both predict and fit_predict exist.

Ohh yeah, now that I've looked through the code I see that anything that inherits from OutlierMixin (at the very least) will have that functionality. But I spent way too long trying to squash a bug because of my misunderstanding of what fit_predict did.

@jnothman
Copy link
Member

@nixphix I don't think there's anything wrong with just creating a pull request for this kind of change

@jnothman jnothman merged commit 39db9c4 into scikit-learn:master Jan 13, 2019
@jnothman
Copy link
Member

Thanks @finnoshea

@finnoshea finnoshea deleted the Mixin_fit_predict_docs branch January 13, 2019 20:27
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 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