Skip to content

[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

Merged
merged 10 commits into from
Feb 12, 2020
Merged

[MRG] Fix Tutorial example code lacking context #13566 #14675

merged 10 commits into from
Feb 12, 2020

Conversation

ngonthier
Copy link
Contributor

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.

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 comments

@ngonthier
Copy link
Contributor Author

Do I have to close the pull request and do a new one with your comments and modifications @glemaitre ?

@jnothman
Copy link
Member

jnothman commented Aug 20, 2019 via email

@ngonthier
Copy link
Contributor Author

But in fact, it seems that @glemaitre commit his modification and that's enough no ?

@glemaitre glemaitre self-assigned this Sep 9, 2019
@glemaitre
Copy link
Member

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.

@glemaitre
Copy link
Member

@adrinjalali The image used in the tutorial used coins. However, we don't install skimage on our CIs. What would be best here to solve the issue of dependencies?

@adrinjalali
Copy link
Member

I'm trying to figure out why it doesn't fail on plot_coin_ward_segmentation.py then. We already use skimage in two examples.

@adrinjalali
Copy link
Member

Kinda wondering why we're testing the docs without installing skimage on azure. We do install that on circleci which is where we test the docs. So basically, our doc build tests (at least on circleci) already has the scikit-image dependency. @thomasjpfan isn't testing the docs in azure redundant when we test them on circlci already?

@glemaitre
Copy link
Member

I'm trying to figure out why it doesn't fail on plot_coin_ward_segmentation.py then. We already use skimage in two examples.

It is run in CircleCI and we install skimage there.

@thomasjpfan
Copy link
Member

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 test_docs.sh to get this to pass doctest.

@adrinjalali
Copy link
Member

@thomasjpfan I genuinely don't know why we test the docs on Azure. Is it because we want to test them in many platforms?

@thomasjpfan
Copy link
Member

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)

@thomasjpfan
Copy link
Member

thomasjpfan commented Oct 29, 2019

I am concerned with how fragile this is. I'll look into this.

Edit: Posted response in wrong issue.

@glemaitre
Copy link
Member

@nicaogr We finally solved the issue with scikit-image #15065. Could you merge the master branch into your branch such that we run the CIs.

@glemaitre
Copy link
Member

Note that we will need to skip the unsupervised_learning.rst file when scikit-image is not installed and that we test the documentation.

@adrinjalali
Copy link
Member

The rendered version looks a bit odd.

@glemaitre
Copy link
Member

I will check and fix it

@glemaitre
Copy link
Member

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

@adrinjalali
Copy link
Member

doc build fails

@glemaitre
Copy link
Member

@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.

@adrinjalali adrinjalali merged commit a3fad52 into scikit-learn:master Feb 12, 2020
@glemaitre
Copy link
Member

Thanks @nicaogr it took a bit of time due to CI but do not hesitate to contribute again.

@ngonthier ngonthier deleted the my-feature branch February 12, 2020 16:33
@ngonthier
Copy link
Contributor Author

Thanks a lot. I will contribute again as soon as possible.

thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020
…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>
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tutorial example code lacking context
5 participants