-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Throw errors when indexing into empty array_view objects #5247
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
Conversation
Should catch issues like matplotlib#5185 while not adding a big perf penalty
to mark empty array views
The tests are failing because of actual problems in |
I think if the array is declared as |
The way you formulated your comment makes me think you misread the code: I didn't change the definition of the |
@jkseppan: I agree with all that. Sorry I wasn't clear. By "declare as empty" I meant "setting m_data to NULL" (like you guessed). |
About performance: I figured the following would be a good microbenchmark, since import matplotlib.image as mimg
import numpy as np
data = np.ones((5000, 5000, 3), dtype=np.uint8)
%timeit mimg.fromarray(data) I'd argue that it's plausible that the checks should add very little delay, since the code has to fetch |
On a Mac 10.10, Python 3.4.3, I get 475ms on master, 883ms on this branch. I did have to make sure to do a clean build to get these results, as the changes are in header files. (At first, I forgot to clean and got results within 10% as you describe). |
Right. I still couldn't replicate that, but it seems that properly cleaning build products is nontrivial. In any case, I made PR #5264 with just the |
Closing in favor of #5274. |
A lower-level complement to #5246, raises an exception when indexing into an
array_view
that includes no data. Should catch issues like #5185 while not adding a big performance penalty in the normal case of nonempty arrays, but of course doesn't catch out-of-bounds errors in that case. I didn't measure the performance impact, but running the tests felt subjectively as fast as usual.