numpy: Handle multidimentional indexing in 1.15 by hmaarrfk · Pull Request #3238 · scikit-image/scikit-image

hmaarrfk · 2018-07-01T23:20:14Z

Handles all numpy deprecations.
~~Goes in an catches the warnings from dependencies that don't follow numpy 1.15~~ PR pytest: Numpy 1 15 warnings #3242.
~~Locks the expected warnings to a specific version.~~ pytest: Numpy 1 15 warnings #3242
~~A few other small fixes where expected_warnings was passed as string as opposed to a list.~~ pytest: Numpy 1 15 warnings #3242
Issue numpy deprecation: Should regular_grid return a tuple? #3235 is handled as a single commit so that it can be rebased if we don't like it.

Future me notes: use

git checkout master
git pull upstream master
git rebase -i master

to pull it out and edit it to whatever the final solution is chosen to be.

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

Clean style in the spirit of PEP8
Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

For reviewers

(Don't remove the checklist below.)

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

pep8speaks · 2018-07-01T23:20:21Z

Hello @hmaarrfk! Thanks for updating the PR.

In the file skimage/util/_regular_grid.py, following are the PEP8 issues :

Line 35:80: E501 line too long (131 > 79 characters)

Comment last updated on July 22, 2018 at 03:24 Hours UTC

hmaarrfk · 2018-07-02T15:28:58Z

Maybe a better way to handle scipy's use of the matrix subclass for sparse matricies is to use https://github.com/pydata/sparse internally.

soupault

Thanks for the PR! I think many of the changes could be simplified to lower the visual complexity of the code. Also, the warnings conditional on package versions should be considered for further refactoring.

soupault · 2018-07-03T14:01:37Z

skimage/restoration/_cycle_spin.py

        futures = [dask.delayed(_run_one_shift)(s) for s in all_shifts]
        mean = sum(futures) / len(futures)
-        mean = mean.compute(get=dask.threaded.get, num_workers=num_workers)
+        mean = mean.compute(scheduler='threading', num_workers=num_workers)


To be fixed via #3205

Ok. That was easy to remove using rebasing :D

soupault · 2018-07-03T14:04:47Z

skimage/segmentation/boundaries.py

    label_img_expanded = np.zeros([(2 * s - 1) for s in label_img.shape],
                                  label_img.dtype)
-    pixels = [slice(None, None, 2)] * ndim
+    pixels = tuple([slice(None, None, 2)] * ndim)


This is just pixels = (slice(None, None, 2), ) * ndim

soupault · 2018-07-03T14:05:53Z

skimage/util/_regular_grid.py

    space_size = float(np.prod(ar_shape))
    if space_size <= n_points:
-        return [slice(None)] * ndim
+        return tuple([slice(None)] * ndim)


soupault · 2018-07-03T14:07:14Z

skimage/util/_regular_grid.py

    slices = [slice(start, None, step) for
              start, step in zip(starts, stepsizes)]
-    slices = [slices[i] for i in unsort_dim_idxs]
+    slices = tuple([slices[i] for i in unsort_dim_idxs])


Why not tuple(slices[i] for i in unsort_dim_idxs)?

You know, I thought you needed the square brackets to make it a list comprehension. I guess you don't :D

@hmaarrfk I have a PEP for you :) - https://www.python.org/dev/peps/pep-0289/

nice one! Yeah, I still don't know generator expressions.

soupault · 2018-07-03T14:07:23Z

skimage/util/arraycrop.py

    ar = np.array(ar, copy=False)
    crops = _validate_lengths(ar, crop_width)
-    slices = [slice(a, ar.shape[i] - b) for i, (a, b) in enumerate(crops)]
+    slices = tuple([slice(a, ar.shape[i] - b)


soupault · 2018-07-03T14:07:31Z

skimage/util/shape.py

    arr_in = np.ascontiguousarray(arr_in)

-    slices = [slice(None, None, st) for st in step]
+    slices = tuple([slice(None, None, st) for st in step])


soupault · 2018-07-03T14:09:40Z

skimage/_shared/_warnings.py

    Finally, you can use `|\A\Z` in a pattern to signify it as optional.

    """
+    if isinstance(matching, str):


This is nice!

soupault · 2018-07-03T14:14:01Z

skimage/filters/lpi_filter.py

    """
    start = (np.array(x.shape) - np.array(oshape)) // 2 + 1
-    out = x[[slice(s, s + n) for s, n in zip(start, oshape)]]
+    out = x[tuple([slice(s, s + n) for s, n in zip(start, oshape)])]


Also here, and below

soupault · 2018-07-03T14:14:39Z

skimage/filters/tests/test_lpi_filter.py

+from skimage._shared._warnings import expected_warnings


+if Version(np.__version__) >= '1.15' and Version(scipy.__version__) <= '1.1.0':


Are there any plans on removing this check in future? If yes, please, add a note to TODO file

soupault · 2018-07-03T14:15:49Z

skimage/restoration/tests/test_denoise.py


        # SNR is improved less with 1 wavelet level than with the default.
-        denoised_1 = restoration.denoise_wavelet(noisy,
+        with expected_warnings([None]):


What is the benefit of using None?

The benefit of using none is that under certain build conditions, where we don't expect warnings, None can be passed in.

hmaarrfk · 2018-07-03T14:58:15Z

@soupault plans for deprecacting the logic behind the conditional warnings:

Well, the plan for removing it is so long term that I didn't know if it makes sense. I'll add some words about it.

I assume that scipy (and pywavelets) will eventually fix these warnings on their side too. at which point, some version of scipy, higher than 1.1.6, will eventually not have these warnings by default. But until then, this is just a very explicit way ensuring warning are

codecov-io · 2018-07-03T15:09:17Z

Codecov Report

Merging #3238 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #3238   +/-   ##
=======================================
  Coverage   81.98%   81.98%           
=======================================
  Files         339      339           
  Lines       27313    27313           
=======================================
  Hits        22392    22392           
  Misses       4921     4921

Impacted Files	Coverage Δ
skimage/morphology/greyreconstruct.py	`96.42% <100%> (ø)`	⬆️
skimage/restoration/_denoise.py	`96.87% <100%> (ø)`	⬆️
skimage/filters/lpi_filter.py	`97.33% <100%> (ø)`	⬆️
skimage/data/_binary_blobs.py	`100% <100%> (ø)`	⬆️
skimage/restoration/uft.py	`100% <100%> (ø)`	⬆️
skimage/feature/template.py	`100% <100%> (ø)`	⬆️
skimage/util/_regular_grid.py	`100% <100%> (ø)`	⬆️
skimage/restoration/tests/test_denoise.py	`99.71% <100%> (ø)`	⬆️
skimage/morphology/extrema.py	`94.89% <100%> (ø)`	⬆️
skimage/util/arraycrop.py	`100% <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 034f505...ad93f97. Read the comment docs.

hmaarrfk · 2018-07-03T15:12:50Z

Finally, I was testing out avoiding the use of |\A\Z since it isn't really explicit as to what conditions might trigger the warning.

Even without instructions, somebody 10 years from now would be able to go through the code, find that a certain combinations of packages can't exist in their supported versions, and rip out the logic if they want to.

That said, I don't know when pywavelets and scipy will address their warnings, maybe it is for us to send in the PR, and the tests might fail once or twice until they get around to fixing their codebase. We would require version bumps on the logic at that point.

The matrix subclass warning should definitely be broadened, as that would require a major change on scipy's end.

hmaarrfk · 2018-07-03T15:15:14Z

I just updated the scipy dependency to not depend on the minor version number for the sparse matrix warning.

soupault

Couple of remarks are still pending, but they aren't blocking :). Otherwise, lgtm.

jni · 2018-07-05T21:50:01Z

Before anyone merges, I suggested in #3235 (comment) that changing the return type of rectangular_grid should be done over a deprecation cycle.

For all except `_regular_grid.py`.

Fixes scikit-image#3235

hmaarrfk · 2018-07-20T19:42:54Z

@jni I finally got around to removing the keyword argument and added your suggested Notes section.

…a tuple add notes in changelog

jni

@hmaarrfk I've tweaked it a little but otherwise LVGTM. ;) Can someone more familiar with Sphinx check my formatting? CC @stefanv

(For some reason I can't build the docs on my machine; make html inside the doc directory gives me the following error:

 ~/projects/scikit-image/doc (numpy_1_15)
 $ make
python tools/build_modref_templates.py
/home/jni/projects/scikit-image/skimage/novice/__init__.py:103: UserWarning: The `skimage.novice` module was deprecated in version 0.14. It will be removed in 0.16.
  warnings.warn("The `skimage.novice` module was deprecated in version 0.14. "
WARNING: Empty - skimage.scripts
outdir:  
27 files written
Build API docs...done.
Copying release notes
python  -b html -d build/doctrees  -j 1 source build/html
python: can't open file 'html': [Errno 2] No such file or directory
Makefile:53: recipe for target 'html' failed
make: *** [html] Error 2

)

hmaarrfk · 2018-07-22T13:01:14Z

@jni I tried:

conda install sphinx-gallery scikit-learn sphinx numpydocs
sudo apt-get install texlive texlive-latex-extra dvipng  # big download

I got pretty far in the building process, but still no cigar. Something about a db mismatch.

soupault · 2018-07-25T19:37:20Z

@jni Looks good to me:

soupault · 2018-07-25T19:40:44Z

Thanks a lot @hmaarrfk !

soupault · 2018-07-25T19:42:18Z

@meeseeksdev backport to v0.14.x

lumberbot-app · 2018-07-25T19:42:27Z

There seem to be a conflict, please backport manually

Handle multidimentional indexing in numpy 1.15

soupault · 2018-07-25T20:09:38Z

Apparently, I have 0 knowledge on the manual backporting of merge commits... Could someone try to handle this, please?

soupault · 2018-07-30T08:24:38Z

(For some reason I can't build the docs on my machine; make html inside the doc directory gives me the following error:

@jni Try to update Sphinx and other docs requirements. Worked for me.

Carreau · 2018-08-13T18:08:01Z

Apparently, I have 0 knowledge on the manual backporting of merge commits... Could someone try to handle this, please?

That's the kind of issue you can raise on MrMeeseeks repo. The "there seem to be a conflict" comments should now have instruction on which git commands to issue to backport a merge commit

Backport of #3238

soupault · 2018-08-17T17:25:11Z

@Carreau Very nice addition, thanks!

hmaarrfk · 2018-08-17T17:45:27Z

I just learned where MrMeeseeks is from....

jni · 2018-08-23T01:49:44Z

@hmaarrfk

I just learned where MrMeeseeks is from....

Time for some Netflix binge-watching. =D

hmaarrfk force-pushed the numpy_1_15 branch from 8e32fcb to fd7a86f Compare July 1, 2018 23:23

hmaarrfk changed the title ~~Handle numpy 1.15 deprecations~~ pytest: Handle numpy 1.15 deprecations to pass tests Jul 1, 2018

soupault reviewed Jul 3, 2018

View reviewed changes

soupault added the 🔧 type: Maintenance Refactoring and maintenance of internals label Jul 3, 2018

soupault added this to the 0.14.1 milestone Jul 3, 2018

hmaarrfk force-pushed the numpy_1_15 branch from fd7a86f to bff86d0 Compare July 3, 2018 15:09

hmaarrfk force-pushed the numpy_1_15 branch from bff86d0 to 9be8867 Compare July 3, 2018 15:14

hmaarrfk force-pushed the numpy_1_15 branch from 9be8867 to f37a395 Compare July 3, 2018 15:25

hmaarrfk mentioned this pull request Jul 3, 2018

pytest: Numpy 1 15 warnings #3242

Merged

4 tasks

hmaarrfk changed the title ~~pytest: Handle numpy 1.15 deprecations to pass tests~~ numpy: Handle multidimentional indexing in 1.15 Jul 3, 2018

soupault approved these changes Jul 3, 2018

View reviewed changes

hmaarrfk force-pushed the numpy_1_15 branch 2 times, most recently from 7b65ff0 to 91a641d Compare July 4, 2018 02:09

hmaarrfk force-pushed the numpy_1_15 branch from 83dc952 to 91a641d Compare July 20, 2018 19:35

hmaarrfk added 2 commits July 20, 2018 15:36

Index numpy arrays with tuples instead of lists

0568261

For all except `_regular_grid.py`.

rectangular_grid returns a tuple for numpy 1.15.

c851576

Fixes scikit-image#3235

hmaarrfk force-pushed the numpy_1_15 branch from 91a641d to c851576 Compare July 20, 2018 19:36

hmaarrfk force-pushed the numpy_1_15 branch from 81fd0ec to 959c00d Compare July 20, 2018 19:47

rectangular_grid: Add a message explaining how to obtain a list from …

e2800a2

…a tuple add notes in changelog

hmaarrfk force-pushed the numpy_1_15 branch from 959c00d to e2800a2 Compare July 20, 2018 21:41

Use versionchanged tag inside output argument docstring

ad93f97

jni approved these changes Jul 22, 2018

View reviewed changes

soupault merged commit 3ffd326 into scikit-image:master Jul 25, 2018

lumberbot-app bot added the Still Needs Manual Backport MrMeeseeks-managed label label Jul 25, 2018

soupault added a commit to soupault/scikit-image that referenced this pull request Jul 25, 2018

Merge pull request scikit-image#3238 from hmaarrfk/numpy_1_15

1e71f79

Handle multidimentional indexing in numpy 1.15

soupault added a commit to soupault/scikit-image that referenced this pull request Jul 25, 2018

Merge pull request scikit-image#3238 from hmaarrfk/numpy_1_15

3026ee7

Handle multidimentional indexing in numpy 1.15

soupault added a commit to soupault/scikit-image that referenced this pull request Jul 25, 2018

Merge pull request scikit-image#3238 from hmaarrfk/numpy_1_15

375c883

Handle multidimentional indexing in numpy 1.15

hmaarrfk mentioned this pull request Aug 1, 2018

Backport #3242: handle NumPy 1.15 warnings in dependencies #3304

Merged

jni mentioned this pull request Aug 2, 2018

Backport of #3238 #3310

Merged

soupault added a commit that referenced this pull request Aug 14, 2018

Merge pull request #3310 from jni/numpy-indexing-2

c4c2f0c

Backport of #3238

hmaarrfk deleted the numpy_1_15 branch November 4, 2018 13:34

		from skimage._shared._warnings import expected_warnings


		if Version(np.__version__) >= '1.15' and Version(scipy.__version__) <= '1.1.0':

Uh oh!

Conversation

hmaarrfk commented Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

References

For reviewers

Uh oh!

pep8speaks commented Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on July 22, 2018 at 03:24 Hours UTC

Uh oh!

hmaarrfk commented Jul 2, 2018

Uh oh!

soupault left a comment

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!

soupault Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Jul 3, 2018

Uh oh!

codecov-io commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hmaarrfk commented Jul 3, 2018

Uh oh!

hmaarrfk commented Jul 3, 2018

Uh oh!

soupault left a comment

Choose a reason for hiding this comment

Uh oh!

jni commented Jul 5, 2018

Uh oh!

hmaarrfk commented Jul 20, 2018

Uh oh!

jni left a comment

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Jul 22, 2018

Uh oh!

soupault commented Jul 25, 2018

Uh oh!

soupault commented Jul 25, 2018

Uh oh!

soupault commented Jul 25, 2018

Uh oh!

lumberbot-app bot commented Jul 25, 2018

Uh oh!

soupault commented Jul 25, 2018

hmaarrfk commented Jul 1, 2018 •

edited

Loading

pep8speaks commented Jul 1, 2018 •

edited

Loading

soupault Jul 3, 2018 •

edited

Loading

codecov-io commented Jul 3, 2018 •

edited

Loading