10000 DOC Fix doc of defaults in sklearn.utils.sparsefuncs.py by franslarsson · Pull Request #18025 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

DOC Fix doc of defaults in sklearn.utils.sparsefuncs.py #18025

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Dec 15, 2020
Merged

DOC Fix doc of defaults in sklearn.utils.sparsefuncs.py #18025

merged 10 commits into from
Dec 15, 2020

Conversation

franslarsson
Copy link
Contributor

Reference Issues/PRs

See #15761

What does this implement/fix? Explain your changes.

This PR change how default values are documented in sklearn.utils.sparsefuncs.py and update docstring according to guideline.

Any other comments?

@rth
Copy link
Member
rth commented Jul 30, 2020

Thanks @franslarsson , there are some linting issues that would need to be resolved.

@franslarsson
Copy link
Contributor Author

Thanks @franslarsson , there are some linting issues that would need to be resolved.

Thanks for pointing it out, the linting issue is solved.

glemaitre
glemaitre reviewed Jul 31, 2020
View reviewed changes
Copy link
Member
@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

A couple of changes to be in line with our documentation guideline.

franslarsson and others added 2 commits July 31, 2020 17:57
@franslarsson @glemaitre
Apply suggestions from code review - added dtype for arrays
153a7a9 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@franslarsson @glemaitre
Apply suggestions from code review - specify format of sparse matrix
4469a3b 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@franslarsson
Copy link
Contributor Author

A couple of changes to be in line with our documentation guideline.

Thank you @glemaitre for the review! I have committed your changes.

@rth
Copy link
Member
rth commented Jul 31, 2020

Valid numpy dtypes are listed in https://numpy.org/doc/stable/user/basics.types.html and float is not one of them. Numpy does convert float type to,

>>> np.dtype(float)
dtype('float64')

but we better write it explicitly as np.float64 (or np.floating if it can be 32bit or 64bit depending on the input or platform).

float is confusing because it's unclear whether it's C float (i.e. 32 bit) or python float (64 bit).

Sorry if this goes in an orthogonal way to @glemaitre 's comments.

@glemaitre
Copy link
Member

Yep I got a bit sloppy there. @franslarsson Could you mention the NumPy types.

@franslarsson
Copy link
Contributor Author

Yep I got a bit sloppy there. @franslarsson Could you mention the NumPy types.

Absolutely, I will do that. I noticed that for example inplace_csr_row_scale(), np.int64, np.float64 and np.float32 seem all to be valid types and give the same results. Should the requirement that dtype=np.float64 still be documented in the docstring or should that be removed in those cases? What do you prefer?

@glemaitre
Copy link
Member

We can stipulate a set of potential type dtype={np.int64, np.float32, np.float64}

@glemaitre
Copy link
Member

I would say that it could be handy for the low-level functions while the high-level functions (usually intended for a user) this might be less interesting and omitting the dtype would be OK. But this is only a personal opinion :)

@franslarsson franslarsson mentioned this pull request Aug 20, 2020
Merged
@glemaitre
Copy link
Member

@franslarsson Could you fix the part regarding the valid numpy dtype?

@franslarsson
Copy link
Contributor Author
franslarsson commented Aug 21, 2020 •
edited
Loading

@franslarsson Could you fix the part regarding the valid numpy dtype?

I have committed one suggestion which includes both 32 and 64. I am a bit unsure how to choose between np.float32 and np.float64 (same for np.int32 and np.int64) so let me know if you have other suggestions. :)

@franslarsson franslarsson mentioned this pull request Aug 30, 2020
Merged
@cmarmo cmarmo mentioned this pull request Sep 24, 2020
Closed
haiatn
haiatn approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor
@haiatn haiatn left a comment

Choose a reason for hiding this comment

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

LGTM

@cmarmo
Copy link
Contributor
cmarmo commented Nov 23, 2020

Hi @franslarsson do you mind fixing conflicts? Then we can push a bit to merge and close #15761. Thanks a lot for your work so far.

@franslarsson
Copy link
Contributor Author

Hi @franslarsson do you mind fixing conflicts? Then we can push a bit to merge and close #15761. Thanks a lot for your work so far.

Hi @cmarmo! I have now fixed the conflicts, sorry for taking so long. Let me know if there is something else that needs to be fixed.

glemaitre
glemaitre approved these changes Dec 7, 2020
View reviewed changes
Copy link
Member
@glemaitre glemaitre 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 PR. LGTM. Could you just remove some blank line that have been added.

@franslarsson
Copy link
Contributor Author

Thanks for the review @glemaitre, I have removed the extra blank lines.

@glemaitre glemaitre merged commit 8e9ced2 into scikit-learn:master Dec 15, 2020
@glemaitre
Copy link
Member

Thanks @franslarsson

We should probably backport this PR in 0.24.X for documentation consistency.

@cmarmo cmarmo added this to the 0.24 milestone Dec 15, 2020
@alfaro96 alfaro96 added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Dec 16, 2020
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Dec 22, 2020
@franslarsson @glemaitre
DOC Fix doc of defaults in sklearn.utils.sparsefuncs.py (scikit-learn…
137684d 
…#18025)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@glemaitre glemaitre mentioned this pull request Dec 22, 2020
Merged
14 tasks
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Apr 22, 2021
@franslarsson @glemaitre
DOC Fix doc of defaults in sklearn.utils.sparsefuncs.py (scikit-learn…
6873365 
…#18025)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

@haiatn haiatn haiatn approved these changes

Assignees
No one assigned
Labels
module:utils To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Milestone
0.24
Development

Successfully merging this pull request may close these issues.
6 participants
@franslarsson @rth @glemaitre @cmarmo @haiatn @alfaro96
0