-
-
Notifications
You must be signed in to change notification settings - Fork 26k
[MRG] Fix Tutorial example code lacking context #13566 #14675
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments
Do I have to close the pull request and do a new one with your comments and modifications @glemaitre ? |
No, please just push additional commits
|
But in fact, it seems that @glemaitre commit his modification and that's enough no ? |
I actually did only comment. I pushed the changes now and trigger the CI to be sure that we don't have issue when running the tutorial. |
@adrinjalali The image used in the tutorial used |
I'm trying to figure out why it doesn't fail on |
Kinda wondering why we're testing the docs without installing |
It is run in CircleCI and we install |
I am +0 on removing the doc test from azure. They are tested on two instances and takes 30 seconds. As for this PR, we can install scikit-image in |
@thomasjpfan I genuinely don't know why we test the docs on Azure. Is it because we want to test them in many platforms? |
This is the only reason I can think of as well. (The azure config is a copy of the original travisci config) |
Edit: Posted response in wrong issue. |
Note that we will need to skip the |
The rendered version looks a bit odd. |
I will check and fix it |
Indeed, this is already broken in master: https://scikit-learn.org/dev/tutorial/statistical_inference/unsupervised_learning.html The display of the image is not proper |
doc build fails |
@adrinjalali it should be ok now. I center the images which take a bit more space than it was intended to but at least this is readable. |
Thanks @nicaogr it took a bit of time due to CI but do not hesitate to contribute again. |
Thanks a lot. I will contribute again as soon as possible. |
…kit-learn#14675) * Add context to tutorial example * try to keep line length under 80 characters * make CI pass * MNT Skips unsupervised_learning when scikit-image is not installed * DOC make code comments as text * solve issue plotting * fix * center images Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com>
…kit-learn#14675) * Add context to tutorial example * try to keep line length under 80 characters * make CI pass * MNT Skips unsupervised_learning when scikit-image is not installed * DOC make code comments as text * solve issue plotting * fix * center images Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com>
Reference Issues/PRs
Fix #13566 : the code was not clear enough in the Connectivity-constrained clustering part of the tutorial.
I replace the code example and I add some comments to explain what we are doing.