Skip to content

DOC Add note about that numpy.matrix is not supported (Fixes: #10993) #20157

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 3 commits into from

Conversation

yoh1496
Copy link

@yoh1496 yoh1496 commented May 28, 2021

Reference Issues/PRs

Fixes #10993. See also #11070.

What does this implement/fix? Explain your changes.

I added note about numpy.matrix is not supported because numpy does not recommend to use.

Any other comments?

Please see #11070 to read discussion about this.

@yoh1496 yoh1496 changed the title DOC: Add note about that numpy.matrix is not supported (Fixes: #10993) DOC Add note about that numpy.matrix is not supported (Fixes: #10993) May 28, 2021
Copy link
Contributor

@cmarmo cmarmo left a comment

Choose a reason for hiding this comment

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

Thanks @yoh1496 for your pull request. I have some minor comments.

Comment on lines 189 to 190
https://numpy.org/doc/stable/reference/generated/numpy.matrix.html
for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is just a matter of taste, but you might want to use a Sphinx external link in this case.
Something like

See `numpy matrix documentation <https://numpy.org/doc/stable/reference/generated/numpy.matrix.html>`_

Comment on lines 474 to 475

def __init__(self, n_clusters=8, *, eigen_solver=None, n_components=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

In general it is a good idea not to add changes unrelated with the pull request: this is confusing when years after we check the commit history.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing out, and sorry about this.
This is made by formatter, so I'll disable and modify this.

@cmarmo
Copy link
Contributor

cmarmo commented May 28, 2021

Also note that the failing check is unrelated with your pull request.

@yoh1496
Copy link
Author

yoh1496 commented May 28, 2021

Thank you for reviewing. I modified codes.

Could you review again?

Copy link
Contributor

@cmarmo cmarmo left a comment

Choose a reason for hiding this comment

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

Thanks @yoh1496! The rendering looks good, also.
Now, an approval by a core developer is needed before merging.

@ogrisel
Copy link
Member

ogrisel commented May 28, 2021

I don't think we should add such a note as part of the docstring because this would be valid for any scikit-learn estimators and we don't want to make all the docstring of all the methods of all scikit-learn estimator that verbose.

Instead, I propose 2.5 things to move forward (should be addressed in distinct PRs):

@cmarmo
Copy link
Contributor

cmarmo commented May 28, 2021

Sorry @yoh1496 it seems that #10993 should take a different path I didn't understand... @ogrisel do you mind commenting directly in the issue? It makes it no longer Easy neither Good first issue.

@ogrisel
Copy link
Member

ogrisel commented Jul 12, 2021

Let's close this. Please feel free to open a new PR to address one of the first two items.

Personally I would be in favor of the second solution: just raise TypeError with an informative error message.

@ogrisel ogrisel closed this Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sklearn.cluster.spectral_clustering fails with np.matrix input
3 participants