Skip to content

Updated the base.OutlierMixin.fit_predict method docstring #12954

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

Merged
merged 1 commit into from
Jan 13, 2019

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