ENH: Deprecation warnings for `/` integer division when running python -3. by njase · Pull Request #8083 · numpy/numpy

njase · 2016-09-23T10:38:24Z

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

charris · 2016-09-23T19:34:25Z

numpy/core/src/umath/ufunc_type_resolution.c

All lines < 80 characters.

Maybe make a list/dict of types and use type_num1 in ...

Accepted, done

charris · 2016-09-23T19:37:29Z

numpy/core/src/umath/ufunc_type_resolution.h

Indentation is off, should line up under PyUFuncObject.

Accepted, corrected in the whole file

seberg · 2016-09-27T07:47:02Z

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.

seberg

Only read the tests stuff, seems like nothing odd came up within numpy (not that I expected it), so mostly nitpicks.

seberg · 2016-09-27T21:27:47Z

numpy/testing/nosetester.py

Two many empty lines.

seberg · 2016-09-27T21:29:49Z

numpy/core/tests/test_regression.py

please put the spaces here as per PEP8 (just nitpicking, but it seemed like a few places, not just here).

Sorry but could you point out to some locations? I could not spot violations (4 spacing rules, max line width, etc are followed)

There should be a space after the comma.

seberg · 2016-09-27T21:33:28Z

numpy/core/tests/test_regression.py

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.

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

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.

seberg · 2016-09-27T21:35:45Z

numpy/core/tests/test_multiarray.py

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.

seberg · 2016-09-28T14:04:11Z

numpy/core/tests/test_multiarray.py

space after the # in PEP8

njase · 2016-09-28T15:57:59Z

All comments taken care. Checks passed. Hope the changes can be accepted now.
Thanks @seberg

seberg · 2016-09-28T17:33:16Z

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 ;).

njase · 2016-09-29T15:32:38Z

I'll squash the commits when there are no more comments, and you give a go ahead.
For release notes: how to decide which version to update?https://github.com/numpy/numpy/tree/master/doc/release

seberg · 2016-09-29T15:54:30Z

numpy/core/src/umath/ufunc_type_resolution.c

There already is PyTypeNum_ISINTEGER which should work.

That was nice :)

seberg · 2016-09-29T15:54:52Z

numpy/core/src/umath/ufunc_type_resolution.c

Nitpick: No spaces on the inside of brackets ;).

Use PyTypeNum_ISINTEGER (and not sure why you got 1 == there).

seberg · 2016-09-29T15:56:57Z

numpy/core/src/umath/ufunc_type_resolution.c

One space missing before the * ;).

seberg · 2016-09-29T16:00:15Z

numpy/core/tests/test_deprecations.py

space after comma :)

Same as earlier.

seberg · 2016-09-29T16:01:07Z

numpy/core/tests/test_deprecations.py

Even here ;) space after comma. Seriously, though, if you don't want to fix, its OK.

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

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).

seberg · 2016-09-29T16:03:08Z

numpy/core/tests/test_regression.py

A bit confused what warning this is, but OK.

I could not add a warning context here as you commented last time. So the idea was to skip this test under -3 switch.

seberg · 2016-09-29T16:06:18Z

numpy/testing/nosetester.py

I might still have a look what the last two things filter later, but I trust you its all good.

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

seberg · 2016-09-29T16:07:19Z

The next release is 1.12, so put it into the 1.12.0 release notes.

seberg · 2016-10-02T15:50:54Z

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).

njase · 2016-10-03T14:44:17Z

Thanks @seberg.
Changes rebased with master
BOOL check added (I learnt it the hard way!) - thanks for pointing out
Should I squash my changes now?

seberg · 2016-10-05T17:50:52Z

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).

charris · 2016-10-06T23:40:07Z

Needs commit squashing, do git rebase -i HEAD~12 and follow directions. After squashing do a force push git push -f origin HEAD.

homu · 2016-10-18T01:37:17Z

☔ The latest upstream changes (presumably 0a02bb6) made this pull request unmergeable. Please resolve the merge conflicts.

njase · 2016-10-30T00:58:41Z

sorry for delay, was quite occupied for last few weeks.
@charris, how do I squash now since my commits are mingled with other's commits when I've merged upstream at least 2 times?

..also, git hub help says for the admin (https://github.com/blog/2141-squash-your-commits"
"Only allow squash commits : This is a new option which lets you force commit squashing on all pull requests merged via the merge button."
Is such an option visible to you for my branch?

stuarteberg · 2016-10-30T19:34:28Z

Hi @njase,

how do I squash now since my commits are mingled with other's commits

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:
https://github.com/stuarteberg/numpy/tree/enh_7949_squashed

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

Appendix

For reference, here's a log of the commands I used to rebase/squash your branch.

I started by making sure that my local numpy repo was in sync with the latest version on github:

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 git rebase again, this time with --interactive mode, which allows you to select commits to combine:

git rebase --interactive master

…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

njase · 2016-11-01T02:06:47Z

Thanks @stuarteberg
Its all done from my side. Ready for inclusion into main release

seberg · 2016-11-01T09:38:52Z

OK, lets give it a shot. Thanks @njase!

njase · 2016-11-01T10:46:36Z

Thanks. I enjoyed working on this and look forward to contribute more in future.

charris reviewed Sep 23, 2016

View reviewed changes

charris changed the title ~~Enh 7949~~ ENH: Deprecation warnings for / integer division when running python -3. Sep 23, 2016

charris added 01 - Enhancement component: numpy._core labels Sep 23, 2016

seberg reviewed Sep 27, 2016

View reviewed changes

seberg reviewed Sep 28, 2016

View reviewed changes

numpy/core/tests/test_multiarray.py Outdated

Copy link
Copy Markdown

Member

seberg Sep 28, 2016

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

stuarteberg mentioned this pull request Sep 28, 2016

Feature request: Emit py3kwarnings for division of integer arrays. #7949

Closed

seberg reviewed Sep 29, 2016

View reviewed changes

njase force-pushed the enh_7949 branch from a585fa1 to 451274c Compare November 1, 2016 01:30

ENH: Deprecation warnings for / integer division when running pytho…

f685f1a

…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

njase force-pushed the enh_7949 branch from 25dea0e to f685f1a Compare November 1, 2016 01:58

seberg merged commit 19fc7ad into numpy:master Nov 1, 2016

njase deleted the enh_7949 branch November 18, 2016 18:46

seberg mentioned this pull request Apr 2, 2018

ENH: Add tester for pytest. #10827

Merged

Uh oh!

Conversation

njase commented Sep 23, 2016

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!

seberg commented Sep 27, 2016

Uh oh!

seberg 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!

njase commented Sep 28, 2016

Uh oh!

seberg commented Sep 28, 2016

Uh oh!

njase commented Sep 29, 2016

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!

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!

seberg commented Sep 29, 2016

Uh oh!

seberg commented Oct 2, 2016

stuarteberg commented Oct 30, 2016 •

edited

Loading