Skip to content

[MRG] DOC: Adding documentation for the argument X in fit() #7784

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
wants to merge 1 commit into from

Conversation

dalmia
Copy link
Contributor

@dalmia dalmia commented Oct 29, 2016

Reference Issue

Addressing #7772

What does this implement/fix? Explain your changes.

This adds a more clear description for the argument X of the function fit() here.

Any other comments?

Since we are computing the centroid, using 'Coordinates' seems more intuitive. Also, this example here helps to understand how X could be visualized as a point in a d-dimensional space.

@dalmia dalmia changed the title Update k_means_.py DOC: Adding documentation for the argument X in fit() Oct 29, 2016
@@ -875,6 +875,7 @@ def fit(self, X, y=None):
Parameters
----------
X : array-like or sparse matrix, shape=(n_samples, n_features)
Coordinates of training data points to cluster
Copy link
Member

Choose a reason for hiding this comment

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

We usually refer to X as the features of the observations, not the coordinates (they could be colors, grades, blood sugar level… pretty much anything).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NelleV Correctly stated. I made a mistake there. How about extending 'sparse matrix with each element representing the feature vector for the training data points to cluster'?

@dalmia dalmia changed the title DOC: Adding documentation for the argument X in fit() [MRG] DOC: Adding documentation for the argument X in fit() Oct 31, 2016
@NelleV
Copy link
Member

NelleV commented Oct 31, 2016

Sorry, this ticket has been addressed in another merged pull request (#7783)

@NelleV NelleV closed this Oct 31, 2016
@amueller
Copy link
Member

amueller commented Oct 31, 2016

I just merged #7783 as it uses a wording that I feel is more common in scikit-learn, and therefore is less surprising. I hope you don't mind. In general, it's good to coordinate with other people that already opened PRs. Thank you for your work!

@dalmia
Copy link
Contributor Author

dalmia commented Oct 31, 2016

Yes Sir, I am just a beginner at open source. Thank you for letting me know. I'll keep that in mind for future issues.

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.

3 participants