BUG: don't check alignment of size=0 arrays (RELAXED_STRIDES) #12511
Conversation
numpy/core/tests/test_api.py
Outdated
| # check RELAXED_STRIDES don't trip assertions in NDEBUG mode (gh-12503) | ||
| buf = np.empty(4, np.uint8) | ||
| buf = buf[2:3][:-1] | ||
| data = np.ndarray((0,), np.double, buf, order='C') |
There was a problem hiding this comment.
Probably should try this for all the affected dtypes.
|
I think the test needs more coverage but otherwise LGTM. Thanks for the quick patch. |
|
The example I provided comes from scipy's test. Specifically I just thought that having a concise direct example would allow to get to the bottom of the issue faster. |
|
That seems like test code worth having in numpy itself. I'll see if I can adapt it here... |
85acd2f to
b91f3ef
Compare
|
Updated with more elaborate test. |
|
Can we / should we update our Travis CI debug run to catch this? I guess we need a few more flags there? |
numpy/core/tests/test_api.py
Outdated
| for align in [1, 2, 3, 4, 8, 16, 32, 64, None]: | ||
| for n in [0, 1, 3, 11]: | ||
| for order in ["C", "F", None]: | ||
| for dtype in [np.uint8, np.float64]: |
There was a problem hiding this comment.
I'd probably test every C type just on general principles, it is a good way to discover unnoticed bugs.
There was a problem hiding this comment.
8000If you made it a test class that would be easier to do, these last loops could be in a test function parametrized by dtype.
|
@tylerjereddy the CI debug run would have caught this if this new test was present. It seems none of our existing tests tested misaligned copies. Thankfully scipy covered for us. |
numpy/core/tests/test_api.py
Outdated
| align = dtype.alignment | ||
| if not hasattr(shape, '__len__'): | ||
| shape = (shape,) | ||
| size = functools.reduce(operator.mul, shape) * dtype.itemsize |
There was a problem hiding this comment.
this can be rewritten as np.prod(shape) * dtype.itemsize
There was a problem hiding this comment.
Gotta be careful of overflow, merged a fix for that just a bit ago that used dtype=np.intp in the prod, although I might just go with int64.
numpy/core/tests/test_api.py
Outdated
| # In particular, check RELAXED_STRIDES don't trip alignment assertions in | ||
| # NDEBUG mode for size-0 arrays (gh-12503) | ||
|
|
||
| def aligned_zeros(shape, dtype=float, order="C", align=None): |
There was a problem hiding this comment.
This function exists in 'test_multiarray', can we refactor the two into one?
There was a problem hiding this comment.
Interestingly, numpy's _aligned_zeros has an extra clause for object types which is missing from scipy's version. Removing it causes a segfault.
So any scipy devs reading this, consider adding that clause.
11f7055 to
483b5cb
Compare
|
Updated to test all dtypes except object. The new test takes 1.8s to run on my system, which is a little long but still acceptable I think. It's a lot of nested loops. |
numpy/core/tests/test_multiarray.py
Outdated
| @@ -7923,6 +7923,44 @@ def test_uintalignment_and_alignment(): | |||
| dst = np.zeros((2,2), dtype='c8') | |||
| dst[:,1] = src[:,1] # assert in lowlevel_strided_loops fails? | |||
|
|
|||
| class TestAlignemnt(object): | |||
|
LGTM check is stuck? |
Don't check alignment of size-0 arrays in copy-loops, because of RELAXED_STRIDES. Fixes numpy#12503
483b5cb to
d7ab72d
Compare
|
Thanks Allan. |
The copy-loops shouldn't assert pointer alignment for size-0 arrays, especially when
RELAXED_STRIDESis turned on.Fixes #12503