Skip to content

[MRG] Removed ref. to deprecated imread/imresize in docs #10502

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 2 commits into from
Jan 19, 2018

Conversation

jotasi
Copy link

@jotasi jotasi commented Jan 19, 2018

Reference Issues/PRs

Fixes: #10499
See also: #10427

What does this implement/fix? Explain your changes.

This removes all occurrences of scipy.misc.imread and scipy.misc.imresize from the docs and examples as these functions are deprecated and will be removed. In the examples they were replaced by sklearn.externals._pilutil.imresize. The mention in the docs was simply removed as they already point to the correct alternatives (skimage and imageio).

Any other comments?

If thats preferred, instead of using sklearn.externals._pilutil.imresize in the two examples examples/cluster/plot_face_segmentation.py and examples/cluster/plot_face_ward_segmentation.py one could also use skimage.transform.rescale.

@jotasi jotasi changed the title Removed ref. to deprecated imread/imresize in docs [MRG] Removed ref. to deprecated imread/imresize in docs Jan 19, 2018
@lesteve
Copy link
Member

lesteve commented Jan 19, 2018

LGTM, merging, thanks a lot!

@lesteve lesteve merged commit 96dd337 into scikit-learn:master Jan 19, 2018
@jnothman
Copy link
Member

jnothman commented Jan 22, 2018 via email

@lesteve
Copy link
Member

lesteve commented Jan 22, 2018

I saw that and I agree that it's not ideal.

  • I am in favour of strongly discouraging people to use our copied and pasted imresize from scipy. Keeping the underscore in _pilutil.py is a good way to do that.
  • the image resizing is only a small non core part of the example (to make it run faster basically) if I believe the comment.

Maybe a more proper way to do it would be to use skimage.transform.resize in our examples (which is the scipy recommendation from their DeprecationWarning)? It seems like a big dependency for such a small function ...

@jotasi
Copy link
Author

jotasi commented Jan 22, 2018

I thought about using skimage.transform.resize (or skimage.transform.rescale for that matter, as this doesn't use a shape but a scale factor its parameter). In the end I decided against it, as the current skimage version (1.13) actually does not include antialiasing as a parameter of rescale (and resize) which seems to be done internally by pillow and thus scipy.misc.pilutil. Therefore, the result actually looks much worse using skimage.transform.rescale. Applying additional antialiasing after rescaling seemed like it would actually distract from the intent of the example. The current dev-version (1.14) does support it as a parameter of rescale and therefore one might be able to switch in the future, though.

To allow for comparison, I appended all three versions of the rescaled image below (scipy.misc.pilutil and skimage.transform.rescale with and without antialiasing):

figure_1

@jnothman
Copy link
Member

jnothman commented Jan 22, 2018 via email

@lesteve
Copy link
Member

lesteve commented Jan 23, 2018

I am not sure I follow your suggestion. Are you saying that the antialiasing is easy to implement once we use skimage.transform.rescale? Not sure where antialised_resize would live, would that be in the example?

If you ask me, it seems like scipy deprecated scipy.misc.imresize without an adequate replacement.

@lesteve
Copy link
Member

lesteve commented Jan 23, 2018

I could be convinced with adding scikit-image as a dependency to the examples as I hinted above. I am not sure where you think antialised_resize should live (maybe in the example?) and how it should be implemented (are you saying it is easy to do the antialising ourselves? I am not very good with image stuff I am afraid).

If you ask me, it seems like scipy deprecated scipy.misc.imresize without a good existing replacement ...

Side comment, it seems like scipy.ndimage.zoom does something similar to skimage.transform.rescale in the current scikit-image stable version (0.13.1). Maybe another option worth considering ...

@jotasi
Copy link
Author

jotasi commented Jan 23, 2018

FWIW, implementation of the antialiasing is very simple. One can simply use the same gaussian filter that sklearn.transform.rescale will be using in the future. It is already already implemented in sklearn.filters.gaussian. Using the values that (AFAICT) will become default in scikit-learn version 0.15, the resizing would become:

import matplotlib.pyplot as plt
from scipy.misc import face
from skimage.transform import rescale
from skimage.filters import gaussian

orig_face = face(gray=True)

# Now blur and then resize:
blurred_face = gaussian(orig_face, sigma=(1/0.1-1)/2, mode="reflect")
rescaled_face = rescale(blurred_face, 0.1, mode="reflect")

plt.imshow(rescaled_face, cmap=plt.cm.gray)
plt.show()

That will result in a properly rescaled image:

figure_1

@jotasi
Copy link
Author

jotasi commented Jan 23, 2018

Concerning scipy.ndimage.zoom: I think it is more suited for actual zooming as it is meant to interpolate between the original pixels. Anyway it is not doing any antialiasing/smoothing AFAICT. I guess a combination of skimage.transform.rescale and skimage.filter.gaussian is the easiest solution. Especially, since in the future one can drop the manual gaussian filtering as soon as skimage's defaults have changed (and that change has reached most installations). Should I open another pull request to drop _pilutil in favor of sklearn?

@lesteve
Copy link
Member

lesteve commented Jan 24, 2018

I am fine using scikit-image in the examples with the two lines you wrote above to do the rescaling with antialiasing. There are a few things you will need to do:

  • check whether it works with scikit-image from Ubuntu 14.04, namely scikit-image 0.9.3. This is the convention we are sticking with at the moment for minimal dependencies. The minimal version of scipy will need to be added to the README.md. If 0.9.3 is too old for some reason, we can probably agree on a more recent minimal version.
  • you need to install scikit-image for the doc generation on CircleCI. This needs to change build_tools/circle/build_doc.sh and in .circleci/config.yml (add the minimal scikit-image version for the Python 2.7 build in .circleci/config.yml)
  • add the link to the CircleCI artifacts once the doc builds so we can easily check that the example looks fine

Open a WIP PR and don't hesitate to ask if you get stuck.

@jotasi jotasi deleted the remove_imread_imresize branch February 15, 2018 13:02
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.

Remove occurences of imread/imresize
3 participants