Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

honno
Copy link
Member

@honno honno commented Aug 23, 2021

No description provided.

@composite
def broadcastable_floating_array_pairs(draw):
dtype = draw(xps.floating_dtypes())
broadcastable_shapes = draw(xps.mutually_broadcastable_shapes(2, min_dims=1))
Copy link
Member

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?

Copy link
Member Author

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))
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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):
Copy link
Member

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).

@asmeurer
Copy link
Member

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).

asmeurer added a commit that referenced this pull request Aug 30, 2021
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.
@honno honno mentioned this pull request Sep 16, 2021
@honno honno closed this Sep 16, 2021
@honno honno deleted the external-strategies branch February 8, 2022 10:05
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

Successfully merging this pull request may close these issues.

2 participants