-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Handle NumPy 1.14 API changes #2935
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
Handle NumPy 1.14 API changes #2935
Conversation
|
There seems to be a lot of issues with the recent numpy release besides the ones fixed via this PR: These are mostly due to the new pretty printing of ndarrays. |
|
As a stop gap measure one can use |
|
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 |
|
@jarrodmillman What was your solution for this in NetworkX? |
|
@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. |
|
@jni ah, indeed, we have one
Would it imply adding a config line to each of the mentioned docstrings? |
|
FYI, here is what we did for nx: |
|
@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. |
|
@oleksandr-pavlyk I think there are several more to address. For example, |
…ith np.issubdtype(dt, np.floating)
There was a problem hiding this 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.
|
It is not clear whether match.py#L57 needs replacing It was probably meant as An instance where turning on the deprecation warning likely caught a genuine bug. |
|
Just in case you didn't find it, you can apply the legacy printing package-wide with this in your It will only apply to the pytest run because pytest imports |
|
@oleksandr-pavlyk it seems that it does not pass for all Travis configurations... |
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
… per deperecation warning
|
Two travis jobs failed with unrelated issues, and 4 passed. XCode build is pending. |
|
|
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefanv Done!
Codecov Report
@@ Coverage Diff @@
## master #2935 +/- ##
=========================================
Coverage ? 86.03%
=========================================
Files ? 337
Lines ? 27050
Branches ? 0
=========================================
Hits ? 23273
Misses ? 3777
Partials ? 0
Continue to review full report at Codecov.
|
|
@Borda tried twice (including Travis cache removal). There seems to be some issues with the pre-release versions of the dependencies. |
|
@soupault do you mean you suggest to merge anyway, despite the problems with Travis? |
|
@emmanuelle yes, that's correct. |
|
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? |
|
@Borda what remains to be done? (in a separate PR) |
|
It seems there is an Shinx issue |
|
How do I find out what is different between invoking the build with no set environment, and with 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 With |
|
Thank you @soupault . Inspecting outputs of
Here is the full list. |
|
The change in Sphinx 1.7 removed support for configuration variable I was only able to find a commented out reference in |
|
In Sphinx 1.7 sources, a plugin for The issue needs to be fixed there. Going to raise an issue. This PR should be merged please. The sooner the better. |
|
In it goes; thanks! |
Borda
left a comment
There was a problem hiding this 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': |
There was a problem hiding this comment.
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
|
although I see requirements that says minimal 1.11 :) |
Yes, that was exactly my thinking. :) |
* 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
A number of test errors result when testing scikit-image with just released NumPy 1.14. The following change fixes the following errors:
The fix is the same as was applied to scikit-learn.