-
Notifications
You must be signed in to change notification settings - Fork 262
BUG: work round int overflow in size calculation #325
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
a6bd532
to
3668cb3
Compare
@@ -504,7 +507,8 @@ def array_from_file(shape, in_dtype, infile, offset=0, order='F', mmap=True): | |||
pass | |||
if len(shape) == 0: | |||
return np.array([]) | |||
n_bytes = int(np.prod(shape) * in_dtype.itemsize) | |||
# Use reduce and mul to work round integer overflow on 32-bit platforms |
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.
s/round/around/g
3668cb3
to
e3beee2
Compare
@@ -504,7 +507,8 @@ def array_from_file(shape, in_dtype, infile, offset=0, order='F', mmap=True): | |||
pass | |||
if len(shape) == 0: | |||
return np.array([]) | |||
n_bytes = int(np.prod(shape) * in_dtype.itemsize) |
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.
You could also specify the dtype
in np.prod
, rather than introduce operator.mul
:
n_bytes = int(np.prod(shape, dtype=np.int64) * in_dtype.itemsize)
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 advantage of using operator.mul
is that is uses Python integers internally, and so completely prevents overflow for any possible value of the shape. In fact the test tests overflow for values above the int64 range, although of course you cannot make an array that large.
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.
Fair enough. Might change the comment to reflect that it's not just avoiding int32 overflows, but that's really nit-picky.
e3beee2
to
4999e50
Compare
Omar pointed out the integer overflow in calculating the image size of an image of size > 4GB, on a 32-bit platform.
OK to merge? |
Not sure who are valid signers-off, but it looks reasonable to me. |
BUG: work round int overflow in size calculation
Thanks @matthew-brett for the fix |
BUG: work round int overflow in size calculation
Omar pointed out the integer overflow in calculating the image size of an
image of size > 4GB, on a 32-bit platform.