-
Notifications
You must be signed in to change notification settings - Fork 45
Multi-dimensional arrays support #17
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
@composite | ||
def broadcastable_floating_array_pairs(draw): | ||
dtype = draw(xps.floating_dtypes()) | ||
broadcastable_shapes = draw(xps.mutually_broadcastable_shapes(2, min_dims=1)) |
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.
Why min_dims=1? We definitely want to make sure dimension 0 arrays are tested.
Actually, can't you use two_mutually_broadcastable_shapes defined above here?
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.
No idea why I did this heh.
dtype = draw(xps.floating_dtypes()) | ||
broadcastable_shapes = draw(xps.mutually_broadcastable_shapes(2, min_dims=1)) | ||
shape1, shape2 = broadcastable_shapes.input_shapes | ||
assume(len(shape1) >= len(shape2)) |
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.
Why is this restriction here?
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.
Ahh this goes back to the issue I was having with the special case tests generating indices which (mostly) worked only when the first array had ndims >=
the second array. This would be resolved if both arrays in the special case tests shared the same shape, as you suggested.
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.
I see. Yeah, there's no need to test broadcasting in the special cases tests (it's already tested separately in test_broadcasting.py), and it just makes building a mask more complicated, so we should just generate arrays of the same shape.
floating_arrays = xps.arrays(dtype=xps.floating_dtypes(), shape=xps.array_shapes()) | ||
|
||
@composite | ||
def broadcastable_floating_array_pairs(draw): |
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.
I think we instead want a function broadcastable_array_pairs
(or mutually_broadcastable_array_pairs
), which takes a dtype strategy as input. This will be similar to the array_scalars
function in this file, which I expect this strategy will replace most instances of. The specific dtype combinations can then be set as floating_arrays = broadcastable_array_pairs(floating_dtypes)
and so on. See the stuff at the top of test_elementwise.py (which should probably be moved to this file).
Now that the NumPy PR is merged I've started taking a look at the test suite again and have taken a closer look at this. It sounds like your PR to get this in hypothesis is going along well, so let's just wait for it to be merged before merging this. That way we can just depend on hypothesis (the good news is hypothesis automatically does a release after each PR merge, so we won't have to wait for that). I don't know how much time you'll have left at that point, but if you want to look at adding this test_elementwise, you can do that. You might have to update some of the tests which are written under the assumption of a dimension-0 input (i.e., you might need to add some masks so that the tests work with full arrays). |
I can't be completely sure that this test is correct until we support testing arbitrary arrays in the elementwise tests (#17), although it should be. This also corresponds to the true type promotion test for equal(), since the test in test_type_promotion.py cannot actually test that equal() does the correct type promotion internally.
No description provided.