[MRG + 1] Remove pilutil from examples #10527

jotasi · 2018-01-24T16:51:33Z

Reference Issues/PRs

See discussion in #10502

What does this implement/fix?

#10502 replaced scipy.misc.imresize, which was deprecated by SciPy, with the local copy sklearn.externals._pilutil.imresize. As pointed out by @jnothman and agreed on in the linked discussion, this usage of a private module in an example is non-ideal. So this PR replaces it again by a combination of scipy.ndimage.filters.gaussian_filter and skimage.image.rescale. The new dependency (scikit-image) is added to the README and to circleCI as pointed out by @lesteve in his instructions on how to proceed (Thank you for these, btw).

Any other comments?

The results differ slightly from the older version. I'll post a comparison later (or tomorrow).
To avoid the overwriting of face from a function to images of different resolution, I introduced new variables orig_face, smoothened_face, and rescaled_face. If preferred, I can also change it back, though.
Different from what was discussed in [MRG] Removed ref. to deprecated imread/imresize in docs #10502, I did not use skimage.filters.gaussian. This wrapper around scipy.ndimage.filters.gaussian_filter changed names from skimage.filter.gaussian_filter to skimage.filters.gaussian_filter in version 0.11 and then to skimage.filters.gaussian in 0.12, while skimage.filters.gaussian_filter was deprecated and is removed in the current dev version 0.14. As the resulting convolution of try: except ImportError: blocks did not seem right for a minor part of a simple example, I went with scipy.ndimage.filters.gaussian_filter instead.
So far I have checked compatibility with the minimum scikit-image version (0.9.3) in the README only in a local docker container. I'll add a link to the circleCI-artifacts as soon as the builds are done.
BTW, while adding scikit-image I saw that the versions in circleCI (SciPy=0.14, Matplotlib=1.3) do not match the minima in the README (SciPy>=0.13.3, Matplotlib>=1.1.1). Is this difference intentional or should this be updated in the README at some point?
Finally, both examples raise a warning on my machine:
matplotlib/contour.py:967: UserWarning: The following kwargs were not used by contour: 'contours'
plot_face_segmentation.py also raises:
sklearn/utils/graph.py:115: FutureWarning: Conversion of the second argument of issubdtype from int to np.signedinteger is deprecated. In future, it will be treated as np.int64 == np.dtype(int).type.
Should I remove the offending parts as well?

jnothman · 2018-01-24T21:06:46Z

You really need to stop referring to skimage as scikit-learn here. It's just too funny. Above you propose we need a new dependency on our own library...

jotasi · 2018-01-24T21:11:43Z

Oh, you're obviously right, shame on me!

jotasi · 2018-01-25T18:52:47Z

Here are the links to the artifacts for python 2 (plot_face_segmentation.py and plot_face_ward_segmentation.py) and python 3 (plot_face_segmentation.py and plot_face_ward_segmentation.py). The links to the examples in the development documentation can be found here and here. I also put example plots of the differences produced on my local machine below, comparing them to the dev-docs. Differences arise for example depending on when the conversion to floats is done and there is also a slight dependence on the boundary conditions. Should I try to tweak the resizing/smoothing to get the results to match more closely or are the results good enough the way they are now?

The resulting differences between before and after this PR on my local machine for plot_face_ward_segmentation.py are similar to those between artifacts and docs:
Now:

Dev-doc:

For plot_face_segmentation.py, the situation is similar. Big differences only arise to the stable doc and that seems to be due to the fix in #9062. Nonetheless, there are still some less pronounced differences:

Now:

Dev-doc:

Update:
Edited out the comparisons to the images before and after #9062 and rather compared the images 1-to-1 with the dev-docs, as suggested by @lesteve. Changed the text accordingly.

lesteve · 2018-01-29T10:53:52Z

BTW, while adding scikit-image I saw that the versions in circleCI (SciPy=0.14, Matplotlib=1.3) do not match the minima in the README (SciPy>=0.13.3, Matplotlib>=1.1.1). Is this difference intentional or should this be updated in the README at some point?

Good catch I updated the README.md for matplotlib and pushed in master (sorry you'll need to fix the conflicts). At the moment we decided to use the versions from Ubuntu 14.04 as our minimal dependencies. We could specify the numpy and scipy version in .circleci/config.yml, if you could do that in a separate PR, that would be great!

For the image comparison you need to compare to the dev doc and not stable. To make it easier to compare, this is better if you post one image for this PR and the same image from the dev doc and then go on to the next image. If you could edit your previous post, that'd be great.

About the image difference, I don't know. Maybe @glemaitre and @jmargeta have some idea since they work on #9062.

Other comments:

it's fine using scipy.ndimage.filters.gaussian_filter
not sure about the warnings, it looks like they should be addressed. If you could do that in a separate PR or open an issue, this would be great.

glemaitre · 2018-01-29T11:00:58Z

About the image difference, I don't know. Maybe @glemaitre and @jmargeta have some idea since they work on #9062.

yep this is due to the fix. I work better now :) The last example (with kmeans) shows the fix. We get cluster which are more consistent.

jmargeta · 2018-01-29T13:33:57Z

I second what @glemaitre says.

@jotasi, it is always good to see it being reproduced, thank you!
The differences not only in graph construction, but also random seed initialization and image type can indeed cause noticeable differences in the boundaries.

It leads me to question whether this image is actually a good example for the technique for a image segmentation / superpixels task. In contrast to papers using similar techniques I do not find the current results particularly convincing (e.g. boundaries are jagged and do not follow the real objects' boundaries). Wouldn't an image with crisper boundaries show the potential more clearly?

lesteve · 2018-01-29T16:14:58Z

I fixed the conflicts. I think this PR should focus on removing sklearn.externals._pilutil usages from the examples. Whether there is better image we can use for this example can be done in a separate issue/PR (and if we go ahead and merge this PR, we can take use one of the scikit-image image)

jotasi · 2018-01-30T05:33:02Z

Thanks for taking care of the conflicts @lesteve, and thanks everyone for the feedback. I've updated my comment above now comparing the 3 images 1-to-1 with the dev-docs. I'll add separate issues/PRs for the following things:

Adding numpy/scipy versions to circleci
Fixing the warnings raised in the example
Checking for a better image

glemaitre · 2018-02-07T22:11:30Z

examples/cluster/plot_face_segmentation.py

@@ -26,26 +26,30 @@

 import numpy as np
 import scipy as sp
+from scipy.ndimage.filters import gaussian_filter


I would use scikit image:

http://scikit-image.org/docs/dev/api/skimage.filters.html#skimage.filters.gaussian

I found that as well. The problem with it is that it changed both name and module at different times since version 0.9.3 (which is the default in Ubuntu trusty) and thus one would need multiple try:... except ImportError:... which did not seem right for a simple example to me. (See the 3rd additional comment in opening post of the PR).
Do you agree that I then should keep SciPys version or should I change it anyway?

glemaitre · 2018-02-07T22:13:16Z

examples/cluster/plot_face_segmentation.py

-face = imresize(face, 0.10) / 255.
+# Applying a Gaussian filter for smoothing prior to down-scaling
+# reduces aliasing artifacts.
+smoothened_face = gaussian_filter(orig_face/255., sigma=(1/0.10-1)/2.)


Actually you don't need that:

http://scikit-image.org/docs/dev/api/skimage.transform.html#skimage.transform.rescale

You can turn anti_aliasing=True to get the same effect.

Actually, this is only in the current dev-version. In the docs of the current stable version (0.13.1) it is not listed, yet. One might want to keep this in mind and change in the (rather far) future, though.
(See also discussion here)

Agreed. In this case rewrite it as:

smoothened_face = gaussian_filter(orig_face, sigma=4.5)

glemaitre · 2018-02-07T22:13:54Z

examples/cluster/plot_face_ward_segmentation.py

-face = imresize(face, 0.10) / 255.
+# Applying a Gaussian filter for smoothing prior to down-scaling
+# reduces aliasing artifacts.
+smoothened_face = gaussian_filter(orig_face/255., sigma=(1/0.10-1)/2.)


see my previous comment

jotasi · 2018-02-08T05:49:24Z

Thanks for reviewing @glemaitre. I've explained my reasoning to your comments inline. Please let me know if you want to change this anyway (or something else).

glemaitre · 2018-02-08T10:44:48Z

I see the point regarding the filtering. Let's keep scipy since that scikit-image is not released.

glemaitre · 2018-02-08T10:50:32Z

examples/cluster/plot_face_segmentation.py

 except ImportError:
-    face = sp.face(gray=True)
+    orig_face = sp.face(gray=True)


to avoid using the / 255, let's use make the following

from skimage import img_as_float64 orig_face = img_as_float64(sp.face(gray=True))

glemaitre · 2018-02-08T10:51:06Z

examples/cluster/plot_face_segmentation.py

+# Applying a Gaussian filter for smoothing prior to down-scaling
+# reduces aliasing artifacts.
+smoothened_face = gaussian_filter(orig_face/255., sigma=(1/0.10-1)/2.)
+rescaled_face = rescale(smoothened_face, 0.10, mode="reflect")


0.10 -> 0.1

glemaitre · 2018-02-08T10:51:18Z

examples/cluster/plot_face_ward_segmentation.py

 except ImportError:
-    face = sp.face(gray=True)
+    orig_face = sp.face(gray=True)


See previous comments

jotasi · 2018-02-08T17:10:52Z

I've implemented your suggestions. My original reasoning for sigma=(1/0.10-1)/2. was that this is the way sigma will be computed by scikit-image's rescale with antialiasing=True in the future but I agree that this is not really helpful without an explanation and as this not the point of the example, I've dropped it. Concerning using skimage.img_as_float64, also good point! Btw. I chose skimage.img_as_float instead, as the former is again only available in the dev-version and skimage.img_as_float produces float64 anyway (see the docs). I hope that this covers everything. Otherwise, please let me know!

glemaitre · 2018-02-08T17:35:23Z

Concerning using skimage.img_as_float64, also good point! Btw. I chose skimage.img_as_float instead, as the former is again only available in the dev-version and skimage.img_as_float produces float64 anyway (see the docs). I hope that this covers everything. Otherwise, please let me know!

Good point. I should really check the stable documentation :)

So this PR LGTM.

@jotasi could you open some issue with the points mentioned there: #10527 (comment)

jotasi · 2018-02-08T18:39:49Z

I've opened an issue for the last point. I'll submit a PR for that if this PR gets merged. The other two should already be fixed by #10557 and #10569 respectively.

jotasi · 2018-02-08T18:47:19Z

Sorry, that was not really true. Actually, the first point

Adding numpy/scipy versions to circleci

is not really fixed, as the PR had to be reverted. It is not possible to simply add the minimum version of SciPy mentioned in the README (0.13.3) to circleCI, as one of the examples actually fails to work with this version (works only for >0.14.0 as explained in this comment in the discussion to #10557). Should I open an issue for that as well as even though it does not seem to be easily fixable?

lesteve · 2018-02-12T16:01:53Z

I pushed some minor tweaks, I will merge when CIs are green.

lesteve · 2018-02-12T16:53:31Z

Circle is green and I checked that the examples looked fine. Merging thanks a lot @jotasi!

Jonathan Tammo Siebert added 4 commits January 24, 2018 16:49

Changed from internal _pilutil to skimage/scipy

24c97db

Updated requirements for the examples

f3ef59a

Added scikit-image needed in examples to circleCI

7de3f0c

gaussian_filter doesn't change dtype

b642431

Merge branch 'master' into remove__pilutil_from_examples

5136106

jotasi changed the title ~~[WIP] Remove pilutil from examples~~ [MRG] Remove pilutil from examples Jan 30, 2018

This was referenced Jan 31, 2018

[MRG] Adding min. req. for numpy and scipy to circleci #10557

Merged

[MRG] contours is no kwarg of matplotlib.pyplot.contour #10569

Merged

glemaitre reviewed Feb 7, 2018

View reviewed changes

glemaitre reviewed Feb 8, 2018

View reviewed changes

Implement glemaitres suggestions

d635051

glemaitre changed the title ~~[MRG] Remove pilutil from examples~~ [MRG + 1] Remove pilutil from examples Feb 8, 2018

jotasi mentioned this pull request Feb 8, 2018

Choose a better image in the examples demonstrating clustering on images #10607

Closed

Minor tweaks

880869c

lesteve merged commit 7f0e433 into scikit-learn:master Feb 12, 2018

jotasi deleted the remove__pilutil_from_examples branch February 15, 2018 13:02

jotasi mentioned this pull request Feb 17, 2018

[MRG] Change image in segmentation example #10647

Merged

Uh oh!

[MRG + 1] Remove pilutil from examples #10527

[MRG + 1] Remove pilutil from examples #10527

Uh oh!

Conversation

jotasi commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix?

Any other comments?

Uh oh!

jnothman commented Jan 24, 2018

Uh oh!

jotasi commented Jan 24, 2018

Uh oh!

jotasi commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Jan 29, 2018

Uh oh!

glemaitre commented Jan 29, 2018

Uh oh!

jmargeta commented Jan 29, 2018

Uh oh!

lesteve commented Jan 29, 2018

Uh oh!

jotasi commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jotasi commented Feb 8, 2018

Uh oh!

glemaitre commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jotasi commented Feb 8, 2018

Uh oh!

glemaitre commented Feb 8, 2018

Uh oh!

jotasi commented Feb 8, 2018

Uh oh!

jotasi commented Feb 8, 2018

Uh oh!

lesteve commented Feb 12, 2018

Uh oh!

lesteve commented Feb 12, 2018

Uh oh!

Uh oh!

jotasi commented Jan 24, 2018 •

edited

Loading

jotasi commented Jan 25, 2018 •

edited

Loading