Skip to content

Conversation

@oleksandr-pavlyk
Copy link
Contributor

A number of test errors result when testing scikit-image with just released NumPy 1.14. The following change fixes the following errors:

'test_rag.test_rag_merge',
'test_rag.test_rag_hierarchical',
'test_rag.test_cut_normalized',
'test_rag.test_threshold_cut',
'skimage.io.tests.test_mpl_imshow.test_signed_image',
'skimage.io.tests.test_mpl_imshow.test_uint8',
'skimage.io.tests.test_mpl_imshow.test_uint16',
'skimage.io.tests.test_mpl_imshow.test_float',
'skimage.io.tests.test_mpl_imshow.test_nonstandard_type',
'skimage.io.tests.test_mpl_imshow.test_outside_standard_range',
'skimage.io.tests.test_mpl_imshow.test_low_data_range',

The fix is the same as was applied to scikit-learn.

@soupault
Copy link
Member

soupault commented Jan 9, 2018

There seems to be a lot of issues with the recent numpy release besides the ones fixed via this PR:


skimage/data/_binary_blobs.py::skimage.data._binary_blobs.binary_blobs FAILED [  6%]
skimage/exposure/exposure.py::skimage.exposure.exposure.histogram FAILED [ 11%]
skimage/exposure/exposure.py::skimage.exposure.exposure.rescale_intensity FAILED [ 11%]
skimage/feature/blob.py::skimage.feature.blob.blob_dog FAILED            [ 15%]
skimage/feature/blob.py::skimage.feature.blob.blob_doh FAILED            [ 15%]
skimage/feature/blob.py::skimage.feature.blob.blob_log FAILED            [ 15%]
skimage/feature/censure.py::skimage.feature.censure.CENSURE FAILED       [ 15%]
skimage/feature/corner.py::skimage.feature.corner.corner_peaks FAILED    [ 16%]
skimage/feature/corner.py::skimage.feature.corner.corner_subpix FAILED   [ 16%]
skimage/feature/corner.py::skimage.feature.corner.structure_tensor FAILED [ 16%]
skimage/feature/corner.py::skimage.feature.corner.structure_tensor_eigvals FAILED [ 16%]
skimage/feature/haar.py::skimage.feature.haar.draw_haar_like_feature FAILED [ 16%]
skimage/feature/orb.py::skimage.feature.orb.ORB FAILED                   [ 16%]
skimage/feature/peak.py::skimage.feature.peak.peak_local_max FAILED      [ 16%]
skimage/feature/template.py::skimage.feature.template.match_template FAILED [ 17%]
skimage/feature/texture.py::skimage.feature.texture.greycoprops FAILED   [ 17%]
skimage/filters/_gaussian.py::skimage.filters._gaussian.gaussian FAILED  [ 29%]
skimage/graph/mcp.py::skimage.graph.mcp.route_through_array FAILED       [ 40%]
skimage/measure/_find_contours.py::skimage.measure._find_contours.find_contours FAILED [ 49%]
skimage/measure/_moments.py::skimage.measure._moments.moments_central FAILED [ 49%]
skimage/measure/_moments.py::skimage.measure._moments.moments_normalized FAILED [ 49%]
skimage/measure/_regionprops.py::skimage.measure._regionprops.regionprops FAILED [ 49%]
skimage/measure/block.py::skimage.measure.block.block_reduce FAILED      [ 49%]
skimage/measure/entropy.py::skimage.measure.entropy.shannon_entropy FAILED [ 49%]
skimage/measure/fit.py::skimage.measure.fit.CircleModel FAILED           [ 50%]
skimage/measure/fit.py::skimage.measure.fit.EllipseModel FAILED          [ 50%]
skimage/measure/fit.py::skimage.measure.fit.LineModelND FAILED           [ 50%]
skimage/measure/fit.py::skimage.measure.fit.ransac FAILED                [ 50%]
skimage/measure/profile.py::skimage.measure.profile.profile_line FAILED  [ 50%]
skimage/measure/tests/test_structural_similarity.py::test_ssim_grad FAILED [ 57%]
skimage/morphology/misc.py::skimage.morphology.misc.remove_small_holes FAILED [ 58%]
skimage/morphology/misc.py::skimage.morphology.misc.remove_small_objects FAILED [ 58%]
skimage/morphology/tests/test_grey.py::test_3d_fallback_white_tophat FAILED [ 62%]
skimage/morphology/tests/test_grey.py::test_3d_fallback_black_tophat FAILED [ 62%]
skimage/segmentation/_join.py::skimage.segmentation._join.relabel_sequential FAILED [ 73%]
skimage/segmentation/boundaries.py::skimage.segmentation.boundaries.find_boundaries FAILED [ 73%]
skimage/transform/_warps.py::skimage.transform._warps.downscale_local_mean FAILED [ 78%]
skimage/transform/integral.py::skimage.transform.integral.integrate FAILED [ 78%]

These are mostly due to the new pretty printing of ndarrays.
Not sure if we should fix them or raise the issue for Sphinx.

@soupault soupault added the 🔧 type: Maintenance Refactoring and maintenance of internals label Jan 9, 2018
@soupault soupault added this to the 0.14 milestone Jan 9, 2018
@soupault soupault self-assigned this Jan 9, 2018
@soupault soupault changed the title fixed NumPy 1.14 deprecation warning Handle NumPy 1.14 API changes Jan 9, 2018
@oleksandr-pavlyk
Copy link
Contributor Author

As a stop gap measure one can use np.set_printoptions(legacy="1.13") in these tests for now. Otherwise doc strings need to be regenerated.

@soupault
Copy link
Member

Nah, I think that is a half measure.

@scikit-image/packaging do you foresee any issues if we update the docstrings in accordance with the new standard? Is skimage being tested in pair with the lower numpy version explicitly somewhere on your end?

@stefanv
Copy link
Member

stefanv commented Jan 11, 2018

@jarrodmillman What was your solution for this in NetworkX?

@jni
Copy link
Member

jni commented Jan 12, 2018

@soupault what do you mean? skimage gets tested with minimal requirements in our CI... The real solution would be to fix doctest to detect equivalences, but that seems improbable. I personally do favour printing in legacy mode until we no longer depend on NumPy <1.14.

@soupault
Copy link
Member

soupault commented Jan 12, 2018

@jni ah, indeed, we have one MINIMUM_REQUIREMENTS=1 build which will not pass in case of such update.

I personally do favour printing in legacy mode until we no longer depend on NumPy <1.14.

Would it imply adding a config line to each of the mentioned docstrings?

@jarrodmillman
Copy link
Contributor

@jni
Copy link
Member

jni commented Jan 13, 2018

@jarrodmillman 2818 is super depressing LOL.

@soupault I really hope that there is an option in pytest to run specific code before each doctest. i don't know if that's true. If not then let's just give up on life. =P Because we have a lot more docstrings depending on numpy than NX does.

@soupault soupault mentioned this pull request Jan 14, 2018
3 tasks
@Borda Borda mentioned this pull request Jan 14, 2018
@soupault
Copy link
Member

@oleksandr-pavlyk I think there are several more to address. For example,

[egor@host scikit-image]$ grep -R issubdtype ./*
...
./skimage/feature/texture.py:    if np.issubdtype(image.dtype, np.float):
...

@soupault soupault changed the title Handle NumPy 1.14 API changes Handle NumPy 1.14 API changes, np.issubdtype Jan 16, 2018
soupault
soupault previously approved these changes Jan 16, 2018
Copy link
Member

@soupault soupault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good.
There are couple of warning related to np.issubdtype (see https://travis-ci.org/scikit-image/scikit-image/jobs/329168966), but they will be addressed within #2888.

@oleksandr-pavlyk
Copy link
Contributor Author

It is not clear whether match.py#L57 needs replacing np.issubdtype(descriptors1.dtype, np.bool) with np.issubdtype(descriptors1.dtype, np.generic) as the warning says it does currently.

It was probably meant as np.issubdtype(descriptors1.dtype, np.bool_) instead:

In [1]: import numpy as np

In [2]: np.issubdtype(np.double, np.bool)
/work/miniconda3/envs/idp_py3_dev/bin/ipython:1: FutureWarning: Conversion of the second argument of issubdtype from `bool` to `np.generic` is deprecated. In future, it will be treated as `np.bool_ == np.dtype(bool).type`.
  #!/localdisk/work/opavlyk/miniconda3/envs/idp_py3_dev/bin/python
Out[2]: True

In [3]: np.issubdtype(np.double, np.bool_)
Out[3]: False

In [4]: np.__version__
Out[4]: '1.14.0'

An instance where turning on the deprecation warning likely caught a genuine bug.

@matthew-brett
Copy link
Contributor

Just in case you didn't find it, you can apply the legacy printing package-wide with this in your conftest.py file at package root:

import numpy
numpy.set_printoptions(legacy='1.13')

It will only apply to the pytest run because pytest imports conftest.py.

@Borda
Copy link
Contributor

Borda commented Jan 16, 2018

@oleksandr-pavlyk it seems that it does not pass for all Travis configurations...

@soupault soupault dismissed their stale review January 17, 2018 13:18

I'm going to finalize the solution within this PR

…_), since the former is interpreted as issubdtype(dt, np.generic) per deprecation warning, clearly unintended here
@oleksandr-pavlyk
Copy link
Contributor Author

Two travis jobs failed with unrelated issues, and 4 passed. XCode build is pending.

@Borda
Copy link
Contributor

Borda commented Jan 19, 2018

FileNotFoundError: [Errno 2] No such file or directory: './sphinx/util/smartypants.py' try to rebuild particular job...

# List of files that pytest should ignore
# Use legacy numpy printing. This fix is made to keep doctests functional.
# For more info, see https://github.com/scikit-image/scikit-image/pull/2935 .
# TODO: remove this workaround once minimal required numpy is set to 1.14.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this to TODO.txt, then we're ready to go.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanv Done!

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@7b40790). Click here to learn what that means.
The diff coverage is 76.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2935   +/-   ##
=========================================
  Coverage          ?   86.03%           
=========================================
  Files             ?      337           
  Lines             ?    27050           
  Branches          ?        0           
=========================================
  Hits              ?    23273           
  Misses            ?     3777           
  Partials          ?        0
Impacted Files Coverage Δ
skimage/io/tests/test_imageio.py 67.39% <0%> (ø)
skimage/io/tests/test_pil.py 88.82% <0%> (ø)
skimage/io/tests/test_simpleitk.py 70.17% <0%> (ø)
skimage/io/tests/test_imread.py 70.58% <0%> (ø)
skimage/io/tests/test_tifffile.py 100% <100%> (ø)
skimage/viewer/plugins/canny.py 100% <100%> (ø)
skimage/morphology/tests/test_grey.py 100% <100%> (ø)
skimage/segmentation/_join.py 96.66% <100%> (ø)
skimage/io/_plugins/matplotlib_plugin.py 75.28% <100%> (ø)
skimage/feature/match.py 96.96% <100%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b40790...017906e. Read the comment docs.

@soupault
Copy link
Member

@Borda tried twice (including Travis cache removal). There seems to be some issues with the pre-release versions of the dependencies.
I'd skip it for now.

@emmanuelle
Copy link
Member

@soupault do you mean you suggest to merge anyway, despite the problems with Travis?

@soupault
Copy link
Member

@emmanuelle yes, that's correct.

@Borda
Copy link
Contributor

Borda commented Jan 22, 2018

I do not feel happy about merging without full pas on Travis, but on the other hand, it is much better to have this mostly fix then nothing... shall we also create a new PR for the rest of required changes?

@emmanuelle
Copy link
Member

@Borda what remains to be done? (in a separate PR)

@Borda
Copy link
Contributor

Borda commented Jan 23, 2018

It seems there is an Shinx issue OSError: [Errno 2] No such file or directory: './sphinx/util/smartypants.py'
but maybe a rebuilding this particular job may be sufficient...
Anyway @oleksandr-pavlyk and @soupault good job, thanks :)

@oleksandr-pavlyk
Copy link
Contributor Author

How do I find out what is different between invoking the build with no set environment, and with PIP_FLAGS="--pre" in terms of package versions to look into the issue?

I created /pull/2947, hoping to see if the phenomenon is specific to changes made in this commit, but errors this PR resolves muddle the picture.

Suggestions?

@oleksandr-pavlyk
Copy link
Contributor Author

Thank you @soupault . Inspecting outputs of pip list executed in before_install.sh L73 for Python 3.5 for default envs and for run with PIP_FLAGS='--pre' only three package versions are different:

  • networkx, 2.0 -> 2.1rc
  • packaging, new
  • Sphinx, 1.6.6 -> 17.0b1

Here is the full list.

PIP_FLAGS=--pre                  |   |  <<no env set>>
===========================================================
alabaster (0.7.10)                      alabaster (0.7.10)
attrs (17.4.0)                          attrs (17.4.0)
Babel (2.5.3)                           Babel (2.5.3)
certifi (2018.1.18)                     certifi (2018.1.18)                 
chardet (3.0.4)                         chardet (3.0.4)
codecov (2.0.14)                        codecov (2.0.14)
coverage (4.4.2)                        coverage (4.4.2)
cycler (0.10.0)                         cycler (0.10.0)
Cython (0.27.3)                         Cython (0.27.3)
dask (0.16.1)                           dask (0.16.1)
decorator (4.2.1)                       decorator (4.2.1)
docutils (0.14)                         docutils (0.14)
flake8 (3.5.0)                          flake8 (3.5.0)
idna (2.6)                              idna (2.6)
imagesize (0.7.1)                       imagesize (0.7.1)
Jinja2 (2.10)                           Jinja2 (2.10)
MarkupSafe (1.0)                        MarkupSafe (1.0)
matplotlib (2.1.1)                      matplotlib (2.1.1)
mccabe (0.6.1)                          mccabe (0.6.1)
networkx (2.1rc1)                  X    networkx (2.0)
numpy (1.14.0)                          numpy (1.14.0)
numpydoc (0.7.0)                        numpydoc (0.7.0)
packaging (16.8)                   X    
Pillow (5.0.0)                          Pillow (5.0.0)
pip (9.0.1)                             pip (9.0.1)
pluggy (0.6.0)                          pluggy (0.6.0)
py (1.5.2)                              py (1.5.2)
pycodestyle (2.3.1)                     pycodestyle (2.3.1)
pyflakes (1.6.0)                        pyflakes (1.6.0)
Pygments (2.2.0)                        Pygments (2.2.0)
pyparsing (2.2.0)                       pyparsing (2.2.0)
pytest (3.3.2)                          pytest (3.3.2)
pytest-cov (2.5.1)                      pytest-cov (2.5.1)
python-dateutil (2.6.1)                 python-dateutil (2.6.1)
pytz (2017.3)                           pytz (2017.3)
PyWavelets (0.5.2)                      PyWavelets (0.5.2)
requests (2.18.4)                       requests (2.18.4)
scipy (1.0.0)                           scipy (1.0.0)
setuptools (38.4.0)                     setuptools (38.4.0)
six (1.11.0)                            six (1.11.0)
snowballstemmer (1.2.1)                 snowballstemmer (1.2.1)
Sphinx (1.7.0b1)                   X    Sphinx (1.6.6)
sphinxcontrib-websupport (1.0.1)        sphinxcontrib-websupport (1.0.1)
toolz (0.9.0)                           toolz (0.9.0)
urllib3 (1.22)                          urllib3 (1.22)
wheel (0.30.0)                          wheel (0.30.0)

@oleksandr-pavlyk
Copy link
Contributor Author

oleksandr-pavlyk commented Jan 24, 2018

The change in Sphinx 1.7 removed support for configuration variable html_use_smartypants. Is it being used?

I was only able to find a commented out reference in doc/source/conf.py#L181.

@oleksandr-pavlyk
Copy link
Contributor Author

In Sphinx 1.7 sources, a plugin for flake8 contains the check for smartypants.py.

The issue needs to be fixed there. Going to raise an issue.

This PR should be merged please. The sooner the better.

@stefanv stefanv merged commit d5de268 into scikit-image:master Jan 26, 2018
@stefanv
Copy link
Member

stefanv commented Jan 26, 2018

In it goes; thanks!

Copy link
Contributor

@Borda Borda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oleksandr-pavlyk @stefanv @soupault
I found an issue in this correction such that any version lover that 1.10 pass throw the condition and crash for unsupported legacy parameter... rather use this

# parse the numpy versions
np_version = [int(i) for i in np.version.full_version.split('.')]
# comparing strings does not work for version lower 1.10
if np_version >= [1, 14]:
    np.set_printoptions(legacy='1.13')

# TODO: remove this workaround once minimal required numpy is set to 1.14.0
import numpy as np

if np.version.full_version >= '1.14.0':
Copy link
Contributor

@Borda Borda Feb 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fails for version lower than 1.10

@Borda
Copy link
Contributor

Borda commented Feb 28, 2018

although I see requirements that says minimal 1.11 :)

@soupault
Copy link
Member

soupault commented Feb 28, 2018

although I see requirements that says minimal 1.11 :)

Yes, that was exactly my thinking. :)

@jni
Copy link
Member

jni commented Mar 1, 2018

@soupault me too, but we should aim to have correct code in our codebase. =) @Borda thanks for the ping!

hmaarrfk pushed a commit to hmaarrfk/scikit-image that referenced this pull request May 18, 2018
* fixed NumPy 1.14 deprecation warning

* replaced remaining instances of deprecated np.issubdtype(dt, float) with np.issubdtype(dt, np.floating)

* repacing issubdtype(dt, np.bool) with intended issubdtype(dt, np.bool_), since the former is interpreted as issubdtype(dt, np.generic) per deprecation warning, clearly unintended here

* issubdtype(dt, 'half') replaced with issubdtype(dt, np.half) per deprecation warning

* replaced issubdtype(dt, np.int) with issubdtype(dt, np.signedinteger) per deperecation warning

* Attempt at replacing use of np.subtract with use of np.logical_xor for arrays of boolean type per NumPy deprecation warning

* Use legacy numpy printing in pytest

* fixed incorrect replacement of np.issubdtype(dt, 'half') with np.issubdtype(dt, np.half). NumPy 1.13 was interperting that as np.issubdtype(dt, np.floating)

* Use view to convert arrays of type np.bool into np.uint8 when passing them to scipy.ndimage functions to work around deprecation warnings in NumPy 1.14 on arithmetic with np.bool arrays

* Added reminedr on pytest doctest workaround to TODO
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🧐 Needs review 🔧 type: Maintenance Refactoring and maintenance of internals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants