[MRG+2]: Copy pilutils functions that are deprecated in scipy #10427

jotasi · 2018-01-08T19:05:07Z

Reference Issues/PRs

Fixes #10147
See also #10149 which is an incomplete and inactive pull request that attempts to fix the same issue.

What does this implement/fix? Explain your changes.

scipy.misc.pilutil's functions are deprecated since version 1.0.0 and will be removed in 1.2.0. As decided in the discussion of #10147 this PR copies the functionality needed in scikit-learn (imread and imresize) and their respective test cases. Following the discussion in #10149 they are put into a utility module in sklearn.util.pilutil. I only included functionality that is already used in scikit-learn, dropping unneeded functions and parameters.

Other comments

I'm still unsure about the following points:

Licensing: As described in SciPy's docs I copied their license's text into the files containing the implementation as well as the tests. Is this sufficient? I'm especially unsure about the images used in the tests which cannot simply include the license text.
Testing: The added tests are not run on Travis as none of the runners installs pillow. Also all other tests that use this functionality are not run. The tests run fine on my machine but it would be nice to add testing on Travis as well. I could add installation of pillow to one/all Travis runners to also execute the tests here. Should this be done or is this unnecessary?

Generally, there were good reasons for SciPy to drop these functions as discussed in the corresponding PR so one might think about following their advice and moving to imageio and maybe skimage in the long run.

jnothman · 2018-01-08T19:50:06Z

This pull request introduces 1 alert - view on lgtm.com

new alerts:

1 for Potentially uninitialized local variable

Comment posted by lgtm.com

jnothman

Maybe this belongs under externals? Otherwise LGTM

jotasi · 2018-01-09T06:59:51Z

Thanks @jnothman for the quick reply. I moved it to externals. I also added Pillow to Travis runners 1 & 2 to verify that the tests are passing there as well. Should I keep this or should I kick it out again?

jnothman

I don't think we want a directory sklearn/externals/tests. Would it hurt to have a sklearn/externals/pilutil/ directory?

jotasi · 2018-01-11T05:42:28Z

Ok. I moved it into its own folder and subpackage. Also I added _imsave, which is used in test_lfw.py. I think now all calls to deprecated scipy.misc.pilutil functions are updated. Does anything else need to be changed?

jnothman · 2018-01-11T07:04:14Z

sklearn/datasets/lfw.py

@@ -31,8 +31,8 @@

 from .base import get_data_home, _fetch_remote, RemoteFileMetadata
 from ..utils import Bunch
+from ..externals.pilutil import _imread, _imresize


How fast is importing pillow/pil on your machine? I would keep the import local if it will slow accessing datasets significantly just because the user happens to have the library installed

On my machine the import from PIL import Image takes approx. 0.04 seconds. The import from sklearn.externals.pilutil import _imread, _imresize takes 0.8 seconds though. Should I keep the global import or should I change it back to a local one?

jnothman

Otherwise lgtm

glemaitre · 2018-01-11T14:58:35Z

build_tools/travis/install.sh

@@ -82,6 +82,10 @@ if [[ "$COVERAGE" == "true" ]]; then
    pip install coverage codecov
 fi

+if [[ "$PILLOW" == "true" ]]; then


Could you merge master because your install.sh is outdated: https://github.com/scikit-learn/scikit-learn/blob/master/build_tools/travis/install.sh#L40

I would probably add it to the TO_INSTALL environment variable if possible

I rebased on the current master and added the environment variable to TO_INSTALL for "$DISTRIB" == "conda". The disadvantage is that I had to separately add it for the "$DISTRIB" == "ubuntu" case. Should I keep it like this?

glemaitre · 2018-01-11T16:23:16Z

‎Would it be a good idea to skip the test for ubuntu and check that an error is raised and use the conda build for the functionality test. @lesteve WDYT?

jotasi · 2018-01-11T16:28:38Z

Atm. the tests are just skipped if pillow is not installed and thus no error would be raised. But I definitely can just add the test to one or both conda builds and skip them on the ubuntu one. My original reasoning for adding it to ubuntu was to check it on both python 2 and 3 (both conda builds are python 3, aren't they?).

glemaitre · 2018-01-11T16:41:02Z

I was counting on appveyor for python 2. The only thing about this ubuntu build is that the version of numpy and scipy are older.

jotasi · 2018-01-12T07:00:34Z

Ok. I changed the last commit accordingly. Should I add pillow to appveyor then?

glemaitre · 2018-01-12T09:24:50Z

yes you can. We were testing this case before (with scipy). I would also do it in circle CI.

jotasi · 2018-01-17T07:14:37Z

As option 3 seems to be the consensus, I've updated the PR: sklearn.externals._pilutil now contains the original versions of all functions used from scipy.misc.pilutil. The only things that were changed are:

When pillow is not installed, the module does not fail at import but when a function is called, that requires pillow.
The documentation was changed slightly and the license was added.
The example for imsave was removed as it would fail on machines without pillow and did not seem to be overly important.

PS: The Travis build is canceled before running not just here, but for all PRs that were updated before. There seems to be something wrong with it. (On my fork, Travis is running fine...)

sklearn-lgtm · 2018-01-17T07:31:28Z

This pull request introduces 1 alert - view on lgtm.com

new alerts:

1 for Potentially uninitialized local variable

Comment posted by lgtm.com

…il.py

lesteve · 2018-01-17T09:51:38Z

I pushed some tweaks, mostly to reduce differences with respect to the original scipy file. I think I am fine with this PR as it is. If someone wants to have another quick look, be my guest.

Probably best to leave it for another PR, but there seems to be some occurences of imread/imresize in doc and examples:

doc/datasets/index.rst:* `scipy.misc.imread <https://docs.scipy.org/doc/scipy/reference/generated/scipy.
doc/datasets/index.rst:  misc.imread.html#scipy.misc.imread>`_ (requires the `Pillow
examples/classification/plot_digits_classification.py:# matplotlib.pyplot.imread.  Note that each image must have the same size. For 
examples/cluster/plot_face_segmentation.py:face = sp.misc.imresize(face, 0.10) / 255.
examples/cluster/plot_face_ward_segmentation.py:face = sp.misc.imresize(face, 0.10) / 255.

sklearn-lgtm · 2018-01-17T11:03:44Z

This pull request introduces 1 alert - view on lgtm.com

new alerts:

1 for Potentially uninitialized local variable

Comment posted by lgtm.com

…into pr/10427

lesteve · 2018-01-17T14:28:48Z

I pulled master to get #10486 which should (hopefully) make Travis happy.

sklearn-lgtm · 2018-01-17T14:59:00Z

This pull request introduces 1 alert - view on lgtm.com

new alerts:

1 for Potentially uninitialized local variable

Comment posted by lgtm.com

jnothman · 2018-01-17T20:27:23Z

I agree with lgtm.com at first glance! It looks like there is a valid code path where strdata is unset!

…

On 18 January 2018 at 01:59, sklearn-lgtm ***@***.***> wrote: This pull request introduces 1 alert - view on lgtm.com <https://lgtm.com/projects/g/scikit-learn/scikit-learn/rev/pr-35a490acdb3ded66c1232633ca8306149794ca3d> *new alerts:* - 1 for Potentially uninitialized local variable ------------------------------ *Comment posted by lgtm.com <https://lgtm.com>* — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10427 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz63T9oT7R9ke05L_nzwx0j7fpi6seks5tLgq2gaJpZM4RWz7J> .

jotasi · 2018-01-18T06:38:54Z

First of all: Thanks @lesteve for cleaning up after me!

Concerning the complaint of lgtm.com:
As far as I can see this cannot happen, although, if there were no restriction to keep the diff small I would add some input validation.
At the beginning of toimage shape is checked to be of either length 2 or 3:
https://github.com/jotasi/scikit-learn/blob/2f78ea7653515be83b837399310bc9621a3feb31/sklearn/externals/_pilutil.py#L357-L361

Therefore, this line, taking the cath element of shape would fail for any ca not in [0,1,2]:
https://github.com/jotasi/scikit-learn/blob/2f78ea7653515be83b837399310bc9621a3feb31/sklearn/externals/_pilutil.py#L410

And thus this block will never be called with ca not in [0,1,2]:
https://github.com/jotasi/scikit-learn/blob/2f78ea7653515be83b837399310bc9621a3feb31/sklearn/externals/_pilutil.py#L415-L423

Although, I do agree that the error message, that the list index of shape is out of range, is not a very informative error message.

Concerning the errors on Travis:
Sadly, the pull of the master did not fix the problem. I think for the ignore option to work one needs to specify the full (absolute or relative) path to the folder/file one wants to ignore, rather than the path from the modules root. At least that is the case on my local machine.

lesteve · 2018-01-18T07:12:37Z

I would ignore the lgtm warning, because that's in externals, for the same reason that we do not test code in externals.

About the error in Travis, looks like the --ignore does not work because we are using the installed sklearn package through --pyargs (and not the local folder). I'll try to figure out a way of doing it. If there is no easy way, removing the doctests in _pilutil.py is reasonable.

lesteve · 2018-01-18T07:29:01Z

I just pushed an alternative solution for not running tests in externals, fingers crossed.

sklearn-lgtm · 2018-01-18T07:59:04Z

This pull request introduces 1 alert - view on lgtm.com

new alerts:

1 for Potentially uninitialized local variable

Comment posted by lgtm.com

…into pr/10427

sklearn-lgtm · 2018-01-18T11:32:29Z

This pull request introduces 1 alert - view on lgtm.com

new alerts:

1 for Potentially uninitialized local variable

Comment posted by lgtm.com

lesteve · 2018-01-18T13:26:55Z

LGTM, merging, thanks a lot @jotasi!

amueller · 2018-01-22T21:25:27Z

Sorry I'm out of the loop, but wouldn't it be better to add a dependency to one of the libraries? That will point people more directly towards the right solution. And we only need this in some rare cases.

jnothman · 2018-01-22T23:22:55Z

What do you mean by adding a dependency to one of the libraries?

glemaitre · 2018-01-23T11:22:46Z

Sorry I'm out of the loop, but wouldn't it be better to add a dependency to one of the libraries? That will point people more directly towards the right solution. And we only need this in some rare cases.

To be able to read image, we will need at least pillow + numpy and make the conversion ourself. This is what imageio is doing in fact. Resizing is a more complex thing since that we would need to use scikit-image which would required dependencies as well.

jotasi force-pushed the copy_deprecated_scikit_pilutils branch 2 times, most recently from bbb3d24 to 3368e30 Compare January 8, 2018 20:51

jnothman reviewed Jan 8, 2018

View reviewed changes

jotasi force-pushed the copy_deprecated_scikit_pilutils branch 4 times, most recently from edea9cf to 71b270b Compare January 9, 2018 06:01

jotasi changed the title ~~[WIP]: Copy pilutils functions that deprecated in scipy~~ [WIP]: Copy pilutils functions that are deprecated in scipy Jan 9, 2018

jnothman reviewed Jan 9, 2018

View reviewed changes

jotasi force-pushed the copy_deprecated_scikit_pilutils branch 7 times, most recently from a930721 to 069e8c9 Compare January 10, 2018 20:42

jnothman reviewed Jan 11, 2018

View reviewed changes

glemaitre reviewed Jan 11, 2018

View reviewed changes

jotasi force-pushed the copy_deprecated_scikit_pilutils branch from 069e8c9 to 4e93951 Compare January 11, 2018 16:01

jotasi force-pushed the copy_deprecated_scikit_pilutils branch from 4e93951 to 276efe6 Compare January 12, 2018 05:52

jotasi force-pushed the copy_deprecated_scikit_pilutils branch from 276efe6 to 0f08bf0 Compare January 12, 2018 16:05

Added back unused parts to reduce diff.

1e851a9

lesteve added 2 commits January 17, 2018 10:42

Tweaks to _pilutil.py to reduce differences from original scipy pilut…

c16a55f

…il.py

Tweak comments

a7e1c26

Minor tweaks

bb89d20

lesteve mentioned this pull request Jan 17, 2018

[MRG] MAINT do not run tests in sklearn/externals #10486

Merged

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

2f78ea7

…into pr/10427

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

b36abe1

…into pr/10427

lesteve force-pushed the copy_deprecated_scikit_pilutils branch from a8d05f9 to b36abe1 Compare January 18, 2018 09:51

lesteve merged commit afe540c into scikit-learn:master Jan 18, 2018

qinhanmin2014 mentioned this pull request Jan 18, 2018

[WIP] Add Utility function to read image from the file as imread is deprecated #10149

Closed

lesteve mentioned this pull request Jan 18, 2018

Remove occurences of imread/imresize #10499

Closed

jotasi deleted the copy_deprecated_scikit_pilutils branch January 18, 2018 15:15

jotasi mentioned this pull request Jan 19, 2018

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

Merged

DimitriPapadopoulos mentioned this pull request Sep 27, 2021

from __future__ import: not needed in Python 3 #21161

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+2]: Copy pilutils functions that are deprecated in scipy #10427

[MRG+2]: Copy pilutils functions that are deprecated in scipy #10427

jotasi commented Jan 8, 2018

jnothman commented Jan 8, 2018

jnothman left a comment

jotasi commented Jan 9, 2018

jnothman left a comment

jotasi commented Jan 11, 2018

jnothman Jan 11, 2018

jotasi Jan 11, 2018

jnothman left a comment

glemaitre Jan 11, 2018

jotasi Jan 11, 2018

glemaitre commented Jan 11, 2018 via email

jotasi commented Jan 11, 2018

glemaitre commented Jan 11, 2018 via email

jotasi commented Jan 12, 2018 •

edited

Loading

glemaitre commented Jan 12, 2018

jotasi commented Jan 17, 2018

sklearn-lgtm commented Jan 17, 2018

lesteve commented Jan 17, 2018 •

edited

Loading

sklearn-lgtm commented Jan 17, 2018

lesteve commented Jan 17, 2018

sklearn-lgtm commented Jan 17, 2018

jnothman commented Jan 17, 2018 via email

jotasi commented Jan 18, 2018 •

edited

Loading

lesteve commented Jan 18, 2018

lesteve commented Jan 18, 2018 •

edited

Loading

sklearn-lgtm commented Jan 18, 2018

sklearn-lgtm commented Jan 18, 2018

lesteve commented Jan 18, 2018

amueller commented Jan 22, 2018

jnothman commented Jan 22, 2018 via email

glemaitre commented Jan 23, 2018

[MRG+2]: Copy pilutils functions that are deprecated in scipy #10427

[MRG+2]: Copy pilutils functions that are deprecated in scipy #10427

Conversation

jotasi commented Jan 8, 2018

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Other comments

jnothman commented Jan 8, 2018

jnothman left a comment

Choose a reason for hiding this comment

jotasi commented Jan 9, 2018

jnothman left a comment

Choose a reason for hiding this comment

jotasi commented Jan 11, 2018

jnothman Jan 11, 2018

Choose a reason for hiding this comment

jotasi Jan 11, 2018

Choose a reason for hiding this comment

jnothman left a comment

Choose a reason for hiding this comment

glemaitre Jan 11, 2018

Choose a reason for hiding this comment

jotasi Jan 11, 2018

Choose a reason for hiding this comment

glemaitre commented Jan 11, 2018 via email

jotasi commented Jan 11, 2018

glemaitre commented Jan 11, 2018 via email

jotasi commented Jan 12, 2018 • edited Loading

glemaitre commented Jan 12, 2018

jotasi commented Jan 17, 2018

sklearn-lgtm commented Jan 17, 2018

lesteve commented Jan 17, 2018 • edited Loading

sklearn-lgtm commented Jan 17, 2018

lesteve commented Jan 17, 2018

sklearn-lgtm commented Jan 17, 2018

jnothman commented Jan 17, 2018 via email

jotasi commented Jan 18, 2018 • edited Loading

lesteve commented Jan 18, 2018

lesteve commented Jan 18, 2018 • edited Loading

sklearn-lgtm commented Jan 18, 2018

sklearn-lgtm commented Jan 18, 2018

lesteve commented Jan 18, 2018

amueller commented Jan 22, 2018

jnothman commented Jan 22, 2018 via email

glemaitre commented Jan 23, 2018

jotasi commented Jan 12, 2018 •

edited

Loading

lesteve commented Jan 17, 2018 •

edited

Loading

jotasi commented Jan 18, 2018 •

edited

Loading

lesteve commented Jan 18, 2018 •

edited

Loading