Skip to content
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

Fix for non-integer indexing in Numpy >= 1.12.0 #203

Closed
mperrin opened this issue Aug 23, 2018 · 6 comments
Closed

Fix for non-integer indexing in Numpy >= 1.12.0 #203

mperrin opened this issue Aug 23, 2018 · 6 comments

Comments

@mperrin
Copy link
Collaborator

mperrin commented Aug 23, 2018

Issue by kmdouglass
Saturday Feb 18, 2017 at 09:54 GMT
Originally opened as mperrin/poppy#203


As of version 1.12.0, NumPy no longer implicitly casts non-integers to integers for array indexing and instead raises an IndexError. See the Numpy Release Notes for details.

A couple of lines in poppy_core.py and the test scripts required either explicit integer casting or integer division with the // operator for compatibility with NumPy >= 1.12.0.

The Poppy test suite is now passing on my machine with Python 3.5.2 and 2.7.13 and Numpy 1.12.0 with these fixes. You may want to add a build with Numpy 1.12.0 to Travis to verify that things work on your end.


kmdouglass included the following code: https://github.com/mperrin/poppy/pull/203/commits

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 23, 2018

Comment by coveralls
Saturday Feb 18, 2017 at 10:07 GMT


Coverage Status

Coverage remained the same at 64.564% when pulling 629488e on kmdouglass:noninteger_index_fix into 802a249 on mperrin:master.

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 23, 2018

Comment by mperrin
Monday Feb 20, 2017 at 20:07 GMT


Very nice, thank you @kmdouglass. I have to admit I hadn't even realized numpy 1.12 was out yet. Much appreciated! I'll give this PR a day or so for code review & feedback from others here (not that it needs much) then pending that I expect this will be good to merge.

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 23, 2018

Comment by kmdouglass
Tuesday Feb 21, 2017 at 05:54 GMT


You're welcome @mperrin . If there's something else that you need me to do, just let me know.

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 23, 2018

Comment by kmdouglass
Wednesday Feb 22, 2017 at 15:39 GMT


Thanks for the feedback. If I understood everyone correctly I will amend this pull request by

  1. removing the comments (I agree with @mperrin that they are a bit too verbose. NumPy makes it very clear why there is an indexing error if these are ever changed in the future);
  2. amend the int(np.abs(np.floor((lx-lx_w)/2))) call to np.abs(lx-lx_w) // 2, leaving the np.abs call because the order of lx and lx_w is not known a priori;
  3. remove the extraneous print statement (I'm sorry about this; I'll be more careful in the future.)

If this all seems correct then I'll make the changes. Thanks guys!

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 23, 2018

Comment by mperrin
Wednesday Feb 22, 2017 at 15:56 GMT


That all sounds correct to me.

(And no need to apologize for an extra print statement - not a big deal. We all do this sort of thing; a big part of why Github is so great is that it makes this kind of code review easy and efficient. All code needs more than one pair of eyes on it!)

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 23, 2018

Comment by coveralls
Wednesday Feb 22, 2017 at 17:21 GMT


Coverage Status

Coverage remained the same at 64.564% when pulling a6ed74c on kmdouglass:noninteger_index_fix into 802a249 on mperrin:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant