pytest: Numpy 1 15 warnings by hmaarrfk · Pull Request #3242 · scikit-image/scikit-image

hmaarrfk · 2018-07-03T15:29:27Z

This PR splits the warning checking from #3238 to allow for a more focused discussion.

Goes in an catches the warnings from dependencies that don't follow numpy 1.15
Locks the expected warnings to a specific version

Also, the warnings conditional on package versions should be considered for further refactoring.

@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

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.

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

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

soupault

There are few extra places where you need to update the format of extected_warnings input argument (see CI).

soupault · 2018-07-03T20:49:13Z

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]):


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

What is, basically, the same thing as with expected_warnings([]): 🤔

Sorry, this one was a rememnent of me testing my None function. I'll rebase and sqaush this.

soupault · 2018-07-03T20:56:57Z

skimage/restoration/tests/test_denoise.py


-            # Verify that SNR is improved with internally estimated sigma
-            denoised = restoration.denoise_wavelet(noisy, method=method)
+            # I don't know why this logic for the warnings is correct


🤣 This comment shouldn't be here.

No, really, this should be removed 🙂 . Or, at least, refactored.

@grlee77 do you have an insight of this?

@soupault I added a little more information that I could gain.

hmaarrfk · 2018-07-03T22:51:08Z

@soupault I'll wait until #3238 is merged in. Some of the fixes needed to happen together.
We get to 0 warnings once we fix that dask deprecation too.

hmaarrfk · 2018-07-04T02:33:25Z

Just a note: scipy will likely silence the matrix 1.2.0

hmaarrfk · 2018-07-04T02:35:59Z

And tuple indexing for scipy seems to be a WIP: scipy/scipy#8944

codecov-io · 2018-07-27T04:04:26Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #3242   +/-   ##
=========================================
  Coverage          ?   86.76%           
=========================================
  Files             ?      339           
  Lines             ?    27363           
  Branches          ?        0           
=========================================
  Hits              ?    23741           
  Misses            ?     3622           
  Partials          ?        0

Impacted Files	Coverage Δ
skimage/measure/tests/test_simple_metrics.py	`100% <100%> (ø)`
skimage/color/tests/test_colorconv.py	`100% <100%> (ø)`
skimage/util/tests/test_dtype.py	`100% <100%> (ø)`
skimage/io/tests/test_imageio.py	`68.75% <100%> (ø)`
skimage/_shared/_warnings.py	`90.9% <75%> (ø)`
skimage/segmentation/tests/test_random_walker.py	`98.43% <95.65%> (ø)`
skimage/filters/tests/test_lpi_filter.py	`87.5% <95.83%> (ø)`
skimage/restoration/tests/test_denoise.py	`99.47% <97.36%> (ø)`

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 dc8b56f...71041da. Read the comment docs.

hmaarrfk · 2018-07-27T14:16:28Z

~~@soupault I don't get it, I swear this should have passed once the other numpy 1.15 was pulled in.~~

~~Unfortunately, it seems that there are some formatting errors now.~~
~~This is for some doctests that don't look like they have been touched in a while.~~

pep8speaks · 2018-07-27T14:32:43Z

Hello @hmaarrfk! Thanks for updating the PR.

In the file skimage/restoration/tests/test_denoise.py, following are the PEP8 issues :

Line 410:80: E501 line too long (84 > 79 characters)

In the file skimage/segmentation/tests/test_random_walker.py, following are the PEP8 issues :

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

Comment last updated on August 01, 2018 at 00:14 Hours UTC

hmaarrfk · 2018-07-27T19:38:30Z

@soupault, sorry for bundling the fix for matplotlib/PyQt5.11 incompatibility.
The goal of this PR is really to re-obtain testing functionality. The commit history is pretty clean and I feel like the temporary fix is also well documented in the script.

Let me know if you disagree, I would gladly separate it as a different PR, but with the builds failing, it is really difficult to ensure correctness.

soupault

@hmaarrfk I'm positive on this PR. Thanks for investigating the remaining issues! Just a couple of simple remarks, otherwise LGTM.

soupault · 2018-07-28T20:29:53Z

skimage/filters/tests/test_lpi_filter.py

+    SCIPY_ND_INDEXING = 'non-tuple sequence for multidimensional'
+else:
+    SCIPY_ND_INDEXING = None
+


https://www.python.org/dev/peps/pep-0008/#blank-lines

soupault · 2018-07-28T20:30:21Z

skimage/filters/tests/test_lpi_filter.py



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


Please, fix the indentation here.

How would you like it to be fixed. Pep8 doesn't specify the "correct" way

# No extra indentation. if (this_is_one_thing and that_is_another_thing): do_something() # Add a comment, which will provide some distinction in editors # supporting syntax highlighting. if (this_is_one_thing and that_is_another_thing): # Since both conditions are true, we can frobnicate. do_something() # Add some extra indentation on the conditional continuation line. if (this_is_one_thing and that_is_another_thing): do_something()

I picked the bottom one.

soupault · 2018-07-28T20:31:19Z

skimage/restoration/tests/test_denoise.py


-            # Verify that SNR is improved with internally estimated sigma
-            denoised = restoration.denoise_wavelet(noisy, method=method)
+            # I don't know why this logic for the warnings is correct


No, really, this should be removed 🙂 . Or, at least, refactored.

soupault · 2018-07-28T20:32:08Z

skimage/restoration/tests/test_denoise.py


-            # Verify that SNR is improved with internally estimated sigma
-            denoised = restoration.denoise_wavelet(noisy, method=method)
+            # I don't know why this logic for the warnings is correct


@grlee77 do you have an insight of this?

soupault · 2018-07-28T20:34:09Z

skimage/restoration/tests/test_denoise.py


    for convert2ycbcr in [True, False]:
        for multichannel in [True, False]:
+            expected = (PYWAVELET_ND_INDEXING if multichannel else None)


I'd refrain from using name expected. Usually, it is assigned to an expected result of the function-under-test.

I called it anticipated_warnings.

soupault · 2018-07-28T20:34:34Z

skimage/segmentation/tests/test_random_walker.py

 PYAMG_SCIPY_EXPECTED = SCIPY_EXPECTED + '|' + PYAMG_EXPECTED_WARNING

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


Indentation here as well.

soupault · 2018-07-28T20:34:54Z

tools/travis/install_qt.sh

 # Now configure Matplotlib to use Qt5
 if [[ "${QT}" == "PyQt5" ]]; then
-    pip install --retries 3 -q $PIP_FLAGS pyqt5
+    # Matplotlib 2.2.2 doesn't support pyqt5 5.11 until this commit


Could you add this to TODO as well?

Monitor when matplotlib releases a version greater than 2.2.2 and test
to see if it becomes compatible with PyQt5.11. If it is compatible,
remove the version limiting logic in tools/travis/install_qt.sh.

This enables us to be more explicit about which warnings we expect depending on dififerent versions.

soupault

Actually, nvm about indentation :). Thanks!

hmaarrfk · 2018-07-30T17:46:27Z

skimage/segmentation/tests/test_random_walker.py

-    expected = ['scipy.sparse.sparsetools|%s' % PYAMG_SCIPY_EXPECTED,
-                NUMPY_MATRIX_EXPECTED]
-    with expected_warnings(expected):
+    anticipated_warnings = [


I thought i caught that one too :/ thanks!

hmaarrfk · 2018-07-30T18:11:41Z

Artifact limit?

jni · 2018-08-01T00:06:05Z

skimage/measure/tests/test_simple_metrics.py

            '`dynamic_range` has been deprecated in favor of '
            '`data_range`. The `dynamic_range` keyword argument '
-            'will be removed in v0.14'):
+            'will be removed in v0.14']):


Erm, why is this still here???? But anyway, I'll leave a fix to a later PR. @soupault?

@jni My bad. :) I'll take care of this.

jni · 2018-08-01T00:17:52Z

Thanks @hmaarrfk! Good stuff. Personally I like expected_warnings to allow a single string (though that wasn't what was happening!), but now it does what the docs always said and anyway it's a matter of debate whether it should accept different input types. Certainly the ability to pass None is very good.

jni · 2018-08-01T00:24:45Z

@meeseeksdev backport to v0.14.x

lumberbot-app · 2018-08-01T00:24:55Z

There seem to be a conflict, please backport manually

hmaarrfk · 2018-08-01T14:00:05Z

@jni, that is certainly a good idea, a single string was convenient. The check could have

if isinstance(matching, str):
   matching = [matching]

I'm not the best at API design, but definitely good at finding places where things aren't working as intended :D

Backport #3242: handle NumPy 1.15 warnings in dependencies

hmaarrfk mentioned this pull request Jul 3, 2018

numpy: Handle multidimentional indexing in 1.15 #3238

Merged

9 tasks

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_warnings branch from 1d092da to 4211ecd Compare July 3, 2018 22:50

hmaarrfk mentioned this pull request Jul 4, 2018

numpy: 1.15 multiindexing warning. targetted fix PyWavelets/pywt#378

Merged

soupault self-assigned this Jul 25, 2018

hmaarrfk force-pushed the numpy_1_15_warnings branch from 849fd15 to 22b6bbb Compare July 27, 2018 04:04

hmaarrfk force-pushed the numpy_1_15_warnings branch from 24fdc0b to 3741ce3 Compare July 27, 2018 15:59

soupault reviewed Jul 28, 2018

View reviewed changes

hmaarrfk force-pushed the numpy_1_15_warnings branch 3 times, most recently from 3c73db4 to 9a3d38a Compare July 28, 2018 22:40

hmaarrfk added 8 commits July 28, 2018 18:41

allow expected_warnings to take entries with None

cbd5968

This enables us to be more explicit about which warnings we expect depending on dififerent versions.

testing: scipy sparse: np.matrix warnings

0eb3c0c

testing: numpy deprecation: pywavelets tuple indexing

23fbf9a

testing: numpy deprecation: scipy doesn't use tuple indexing

f2a9ec7

todo: numpy deprecation: added instructions deprecate logic for 1.15.0

041e83f

testing: warning parameters should be lists, not strings

faa9afd

testing: numpy deperecation: fixed version pinning for warnings

060ccdd

Temporary fix for PyQt5.11 matplotlib 2.2 incompatibility

cb74435

hmaarrfk force-pushed the numpy_1_15_warnings branch 2 times, most recently from 415a996 to c60ae9a Compare July 30, 2018 03:13

hmaarrfk force-pushed the numpy_1_15_warnings branch 2 times, most recently from aedca22 to cb74435 Compare July 30, 2018 15:44

Renamed variable in random walker tests

d46ddce

soupault approved these changes Jul 30, 2018

View reviewed changes

soupault added the status: mrg+1 label Jul 30, 2018

hmaarrfk commented Jul 30, 2018

View reviewed changes

Fix typo

e6304fe

jni reviewed Aug 1, 2018

View reviewed changes

Rename warning constants for better readability

71041da

jni merged commit 259ea05 into scikit-image:master Aug 1, 2018

lumberbot-app bot added the Still Needs Manual Backport MrMeeseeks-managed label label Aug 1, 2018

jni added a commit to jni/scikit-image that referenced this pull request Aug 1, 2018

Backport scikit-image#3242: handle NumPy 1.15 warnings in dependencies

4b04f4a

This was referenced Aug 1, 2018

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

Merged

Minor PEP8 fixes #3305

Merged

jni added a commit to jni/scikit-image that referenced this pull request Aug 15, 2018

Backport scikit-image#3242: handle NumPy 1.15 warnings in dependencies

36f1466

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

Merge pull request #3304 from jni/backport-3242

c8443d8

Backport #3242: handle NumPy 1.15 warnings in dependencies

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

jni mentioned this pull request Dec 1, 2018

Use fused types in denoise, warp #3486

Merged

9 tasks



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

Uh oh!

Conversation

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

Uh oh!

For reviewers

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!

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!

hmaarrfk commented Jul 3, 2018

Uh oh!

hmaarrfk commented Jul 4, 2018

Uh oh!

hmaarrfk commented Jul 4, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Comment last updated on August 01, 2018 at 00:14 Hours UTC

Uh oh!

hmaarrfk commented Jul 27, 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jni commented Aug 1, 2018

Uh oh!

hmaarrfk commented Jul 3, 2018 •

edited

Loading

soupault Jul 3, 2018 •

edited

Loading

codecov-io commented Jul 27, 2018 •

edited

Loading

hmaarrfk commented Jul 27, 2018 •

edited

Loading

pep8speaks commented Jul 27, 2018 •

edited

Loading