The Wayback Machine - https://web.archive.org/web/20250523011721/https://github.com/scikit-learn/scikit-learn/pull/18360
Skip to content

DOC Fix documentation of default values in sklearn.feature_extraction.text.py #18360

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 17 commits into from
Sep 24, 2020
Merged

DOC Fix documentation of default values in sklearn.feature_extraction.text.py #18360

merged 17 commits into from
Sep 24, 2020

Conversation

haiatn
Copy link
Contributor

@haiatn haiatn commented Sep 8, 2020

….text.py

Reference Issues/PRs

Partially addresses #15761

What does this implement/fix? Explain your changes.

two parameters of token_pattern have a default value so I added it.

Any other comments?

@haiatn
Copy link
Contributor Author

haiatn commented Sep 8, 2020

Hi, I'm still new to this,
do you understand why it's failing?
It's a linting issue. The last time I had this I needed to add a line but it didn't work for me this time.

glemaitre
glemaitre reviewed Sep 8, 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.

I think it would be worth to correct all the docstring of the module using the doc guideline

@haiatn
Copy link
Contributor Author

haiatn commented Sep 8, 2020 •
edited
Loading

I didn't understand at first how to work with backslash inside a docstring so that caused some problems, but now it works.

@haiatn haiatn requested a review from rth September 9, 2020 17:22
@haiatn haiatn mentioned this pull request Sep 18, 2020
Closed
alfaro96
alfaro96 reviewed Sep 20, 2020
View reviewed changes
Copy link
Member

@alfaro96 alfaro96 left a comment

Choose a reason for hiding this comment

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

Hey @cmarmo,

IIRC, you are working on #18377 how to specify the token_pattern parameter for proper rendering, is is not?

@haiatn
Copy link
Contributor Author

haiatn commented Sep 20, 2020 •
edited
Loading

oh yes, it seems we reached the same conclusion. The only change between the pull requests I see is adding r prefix in r''' instead of ''' in the docstring. I am not sure it is needed. should I close this?

@alfaro96
Copy link
Member

oh yes, it seems we reached the same conclusion. The only change between the pull requests I see is adding r prefix in r''' instead of ''' in the docstring. I am not sure it is needed. should I close this?

The main difference is how the regular expression is rendered in the documentation:

I would not close this PR, changing string by str is still needed to follow the documentation guidelines.

@cmarmo
Copy link
Contributor

cmarmo commented Sep 20, 2020

@alfaro96 , @haiatn , I'm fine if you take over all the modifications: I think that @thomasjpfan and I agreed that raw docstring is a good way to solve the rendering issue and this PR could be merged quickly... :)
Thanks @haiatn for your work!

@cmarmo cmarmo mentioned this pull request Sep 20, 2020
Closed
thomasjpfan
thomasjpfan reviewed Sep 22, 2020
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Small comment on analyzer, otherwise LGTM.

thomasjpfan
thomasjpfan approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan merged commit 896d4fa into scikit-learn:master Sep 24, 2020
@cmarmo cmarmo mentioned this pull request Sep 24, 2020
Closed
@haiatn haiatn deleted the pr/last branch September 24, 2020 16:36
amrcode pushed a commit to amrcode/scikit-learn that referenced this pull request Oct 19, 2020
@haiatn @rth
@glemaitre @thomasjpfan @amrcode
DOC Fix documentation of default values in sklearn.feature_extraction…
9e2cf13 
….text.py (scikit-learn#18360)

Co-authored-by: Roman Yurchak <[email protected]>
Co-authored-by: Guillaume Lemaitre <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@haiatn @rth
@glemaitre @thomasjpfan @jayzed82
DOC Fix documentation of default values in sklearn.feature_extraction…
d6b1a9b 
….text.py (scikit-learn#18360)

Co-authored-by: Roman Yurchak <[email protected]>
Co-authored-by: Guillaume Lemaitre <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre left review comments

@alfaro96 alfaro96 alfaro96 left review comments

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@rth rth Awaiting requested review from rth

Assignees
No one assigned
Labels
Documentation module:feature_extraction
Projects
None yet
Milestone
No milestone
Development

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