Skip to content

MAINT: consistent use of print(__doc__) in examples #21307

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 3 commits into from
Oct 22, 2021

Conversation

DimitriPapadopoulos
Copy link
Contributor

What does this implement/fix? Explain your changes.

Run print(__doc__) right after the example docstring.

As suggested in #21246 (review).

Any other comments?

@thomasjpfan
Copy link
Member

All files under examples/release_highlights lack a print(doc) so I haven't added it (yet). Should I?

I would say include them to be consistent. (Although it does not look as nice having a print(__doc__) rendered on the HTML)

The files under examples/text print doc later in the code, together with additional usage information. Not sure I should change them.

In those cases, I think it is best to leave them as is.

Copy link
Member

@TomDLT TomDLT left a comment

Choose a reason for hiding this comment

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

All good, thanks !

@adrinjalali
Copy link
Member

So historically, they were there since we'd have pretty much all of the text on top of the file, and then the print would print all the information related to the example. But these days our examples are sectioned, and suited for a nice sphinx rendering and notebooks, and users can run them in binder. I'd say it'd be nicer if we remove them from all the files.

@DimitriPapadopoulos
Copy link
Contributor Author

Both are OK with me. I just need a wider agreement on what is the best solution. Can I help reach it?

@adrinjalali
Copy link
Member

pinging @scikit-learn/core-devs since we kinda need an agreement on this.

Question is: should we remove print(__doc__) from all examples or should we add it to all of them?

@NicolasHug
Copy link
Member

I'd tend towards removing as well.

The printing is useful if one blindly runs the example from the command line without even looking at the code, but I'm not sure many people do (or even should) do that.

@GaelVaroquaux
Copy link
Member

GaelVaroquaux commented Oct 14, 2021 via email

@glemaitre
Copy link
Member

I don't have a strong opinion for or against removing.

@agramfort
Copy link
Member

agramfort commented Oct 14, 2021 via email

@thomasjpfan
Copy link
Member

thomasjpfan commented Oct 14, 2021

I prefer to remove it.

@DimitriPapadopoulos
Copy link
Contributor Author

I have removed the initial __print__(doc) from all examples so that you can have a look.

Only exception, the 3 examples under examples/text. They print(__doc__) late in the source file, together with additional usage information.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

nice work!

@glemaitre glemaitre self-requested a review October 22, 2021 12:29
@glemaitre
Copy link
Member

glemaitre commented Oct 22, 2021

I just merged master and we can check if all CIs become green.

@glemaitre glemaitre merged commit d4d5f8c into scikit-learn:main Oct 22, 2021
@glemaitre
Copy link
Member

Thanks @DimitriPapadopoulos

@DimitriPapadopoulos DimitriPapadopoulos deleted the print__doc__ branch October 22, 2021 14:36
@glemaitre glemaitre mentioned this pull request Oct 23, 2021
10 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Oct 23, 2021
)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
glemaitre added a commit that referenced this pull request Oct 25, 2021
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
)

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants