-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
ENH: Deprecation warnings for /
integer division when running python -3.
#8083
Conversation
|
||
/* If both types are integer, warn the user, same as python does */ | ||
if ( Py_DivisionWarningFlag && | ||
( type_num1 == NPY_BOOL || type_num1 == NPY_BYTE || type_num1 == NPY_UBYTE || \ |
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.
All lines < 80 characters.
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.
Maybe make a list/dict of types and use type_num1 in ...
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.
Accepted, done
@@ -65,6 +65,13 @@ PyUFunc_MultiplicationTypeResolver(PyUFuncObject *ufunc, | |||
PyArray_Descr **out_dtypes); | |||
|
|||
NPY_NO_EXPORT int | |||
PyUFunc_MixedDivisionTypeResolver(PyUFuncObject *ufunc, | |||
NPY_CASTING casting, |
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.
Indentation is off, should line up under PyUFuncObject
.
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.
Accepted, corrected in the whole file
/
integer division when running python -3.
Maybe a start: diff --git a/numpy/testing/nosetester.py b/numpy/testing/nosetester.py
index c30c0ee..87bc344 100644
--- a/numpy/testing/nosetester.py
+++ b/numpy/testing/nosetester.py
@@ -412,6 +412,17 @@ class NoseTester(object):
from ..distutils import cpuinfo
sup.filter(category=UserWarning, module=cpuinfo)
+ # Filter out deprecation warnings due to the -3 flag to python 2.
+ if sys.version_info.major == 2 and sys.py3kwarning:
+ # This is very specific, so using the fragile module filter
+ # is fine.
+ import threading
+ sup.filter(DeprecationWarning,
+ r"sys\.exc_clear\(\) not supported in 3\.x",
+ module=threading)
+ sup.filter(DeprecationWarning, message="in 3\.x, __setslice__")
+ sup.filter(DeprecationWarning, message="in 3\.x, __getslice__")
+
# Filter out some deprecation warnings inside nose 1.3.7 when run
# on python 3.5b2. See
# https://github.com/nose-devs/nose/issues/929 If the slice things comes from a single module, could use a module filter. But my guess is it probably does not. |
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.
Only read the tests stuff, seems like nothing odd came up within numpy (not that I expected it), so mostly nitpicks.
sup.filter(DeprecationWarning, message="buffer\(\) not supported in 3\.x") | ||
sup.filter(DeprecationWarning, message="CObject type is not supported in 3\.x") | ||
|
||
|
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.
Two many empty lines.
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.
Done
@@ -2115,6 +2115,10 @@ def __array__(self,*args,**kwargs): | |||
for f in [op.lt, op.le, op.gt, op.ge]: | |||
if sys.version_info[0] >= 3: | |||
assert_raises(TypeError, f, lhs, rhs) | |||
elif sys.version_info[0] < 3 and sys.py3kwarning: | |||
warnings.filterwarnings('error',category=DeprecationWarning) |
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 put the spaces here as per PEP8 (just nitpicking, but it seemed like a few places, not just here).
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.
Sorry but could you point out to some locations? I could not spot violations (4 spacing rules, max line width, etc are followed)
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.
There should be a space after the comma.
elif sys.version_info[0] < 3 and sys.py3kwarning: | ||
warnings.filterwarnings('error',category=DeprecationWarning) | ||
assert_raises(DeprecationWarning, f, lhs, rhs) | ||
warnings.filterwarnings('ignore',category=DeprecationWarning) |
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.
The test is failing due to this. Don't use "ignore" filters, they are horribly broken in versions before python 3.4.x if you want to reliably test them. Also filters should always be inside a context (either catch_warnings, or our own suppress_warnings context).
My guess is: put this test into a warnings context. Then add a new test if you want to check whether it actually gives the warning when py3kwarning is active.
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.
Hmm, I see. I also wondered why so many tests in a single file are unexpectedly failing.
On second thought, should I just ignore this test during -3 switch?
The test will anyhow run without -3 as part of the whole test suite..?
All other comments I'll take care asap
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.
Since the tests run anyhow, I am OK with skipping when its easier. Though suppression is cleaner and seems preferable to me if it is straight forward.
@@ -6457,7 +6457,7 @@ class TestArrayPriority(TestCase): | |||
op.ge, op.lt, op.le, op.ne, op.eq | |||
] | |||
|
|||
if sys.version_info[0] < 3: | |||
if sys.version_info[0] < 3 and not sys.py3kwarning: |
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.
Could maybe use a comment. Or put the failing tests into a suppress_warnings context. I think I am fine with just doing it like this, but an explanation might help in the long run.
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.
Accepted
@@ -6457,6 +6457,8 @@ class TestArrayPriority(TestCase): | |||
op.ge, op.lt, op.le, op.ne, op.eq | |||
] | |||
|
|||
#See #7949. Dont use "/" operator With -3 switch, since python reports it | |||
#as a DeprecationWarning |
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.
space after the #
in PEP8
All comments taken care. Checks passed. Hope the changes can be accepted now. |
You are too fast :). I still have to read all, so no need to do all yet, but should still add a release note and squash the commits. Thanks for bwing patient with the nitpicks though ;). |
I'll squash the commits when there are no more comments, and you give a go ahead. |
@@ -1210,6 +1211,67 @@ type_reso_error: { | |||
} | |||
} | |||
|
|||
int is_integer_type(int type_num) |
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.
There already is PyTypeNum_ISINTEGER
which should work.
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.
That was nice :)
/* If both types are integer, warn the user, same as python does */ | ||
if ( Py_DivisionWarningFlag && | ||
(1 == is_integer_type(type_num1)) && | ||
(1 == is_integer_type(type_num2)) ) |
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.
Nitpick: No spaces on the inside of brackets ;).
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.
Use PyTypeNum_ISINTEGER (and not sure why you got 1 ==
there).
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.
accepted
* See PEP238 and #7949 for numpy | ||
* This function will not be hit for py3 or when __future__ imports division. | ||
* See generate_umath.py for reason | ||
*/ |
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.
One space missing before the *
;).
import operator as op | ||
for dt in deprecated_types: | ||
a = np.array([1,2,3], dtype=dt) | ||
self.assert_deprecated(op.div, args=(a,a)) |
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.
space after comma :)
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.
Same as earlier.
if sys.version_info[0] < 3 and sys.py3kwarning: | ||
import operator as op | ||
for dt in deprecated_types: | ||
a = np.array([1,2,3], dtype=dt) |
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.
Even here ;) space after comma. Seriously, though, if you don't want to fix, its OK.
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.
PEP8 discourages extraneous whitespace immediately before comma,semicolon (see second bullet in section Pet Peeves)
In this case, space is after comma which should be acceptable since it avoids cluttering (IMHO)
Please correct me if I am wrong. For consistency and code quality, I am ready to change my coding habits :)
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.
Don't worry about it, I meant within the list, which sometimes is really OK to not put the space after comma. Not sure, I can imagine PEP8 says no space after the comma is OK in some cases (e.g. I also sometimes tend to skip it in indexing operations).
else: | ||
elif not sys.py3kwarning: | ||
# With -3 switch in python 2, DeprecationWarning is raised | ||
# which we are not interested in |
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.
A bit confused what warning this is, but OK.
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.
I could not add a warning context here as you commented last time. So the idea was to skip this test under -3 switch.
sup.filter(DeprecationWarning, message="in 3\.x, __getslice__") | ||
sup.filter(DeprecationWarning, message="buffer\(\) not supported in 3\.x") | ||
sup.filter(DeprecationWarning, message="CObject type is not supported in 3\.x") | ||
sup.filter(DeprecationWarning, message="comparing unequal types not supported in 3\.x") |
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.
I might still have a look what the last two things filter later, but I trust you its all good.
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.
With the existing unit tests (before this change), overall 6 types of errors were reported with -3 switch. Those reported as "classic int division" have been fixed.
- "CObject.." API are deprecated, so I ignored such errors.
- "unequal types..". I could not figure out how these cases pass with actual python 3 version. But they do, so I ignored them with -3 switch
The next release is 1.12, so put it into the 1.12.0 release notes. |
Your probably already got it, but you will have to rebase your changes on master to avoid conflicts in the release notes (unfortunatly, those happen quite quickely). I also think you may have check for the BOOL typenum as well (there is also a macro for that). |
Thanks @seberg. |
Thanks @njase, I think I am done with nitpicking. If you squash it I will merge it (unless someone else has an opinion about it). |
Needs commit squashing, do |
☔ The latest upstream changes (presumably 0a02bb6) made this pull request unmergeable. Please resolve the merge conflicts. |
sorry for delay, was quite occupied for last few weeks. ..also, git hub help says for the admin (https://github.com/blog/2141-squash-your-commits" |
Hi @njase,
On my machine, I was able to rebase your branch on the latest master and also squash all the commits into one. (See appendix, below.) I pushed the results to a branch on my fork: If that looks okay to you, you can update this PR as follows: git checkout enh_7949
git branch backup_enh7949 # Save a copy of your branch, just in case
git remote add stuart https://github.com/stuarteberg/numpy
git reset --hard stuart/enh_7949_squashed
git push -f https://github.com/njase/numpy enh_7949 AppendixFor reference, here's a log of the commands I used to rebase/squash your branch. I started by making sure that my local cd numpy
git checkout master
git pull origin master # My 'origin' is the main numpy repo: github.com/numpy/numpy Then I fetched and checked out your branch: git remote add saurabh ssh://git@github.com/njase/numpy
git fetch saurabh
git checkout enh_7949 Then I rebased your branch on the latest commit from master: git rebase master
# (Here, I had to fix one tiny merge conflict during the rebase operation.)
git mergetool
git rebase --continue At this point, the git history looks as if all of your commits were just now added, linearly, on top of today's master branch. To squash them into a single commit, I used
|
…n -3 When python is invoked with switch -3, it emits waring "classic int division" for strict integer divisions. The same behavior is now implemented to numpy with this fix
Thanks @stuarteberg |
OK, lets give it a shot. Thanks @njase! |
Thanks. I enjoyed working on this and look forward to contribute more in future. |
This feature implements deprecation warning mechanism for integer division in numpy inline with python -3
See PEP 238 for Python's behaviour, summarised as:
before 2.2
Operator // does not exist
Operator / exists and has mixed behaviour. For integers its floor division, otherwise best possible
2.2 onwards until 3.0
Operator // is now defined for floor division irrespective of numerical data type
Operator / - same as earlier
Using -3 switch will report warning if mismatch with future (3.x) behaviour
Starting from 3.0
Operator // for floor division, same as earlier
/ best possible division