Try using hypothesis.target() to better test BooleanArrays #78
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…edError These only work in NumPy as fancy indices. However, there is too much of a risk of API confusion in thinking that Tuple() works like Tuple((1, 2, 3)) instead of Tuple(1, 2, 3). So we make this an unconditional error with helpful error messages. Fancy indices should use either a list or array. c.f. issue Quansight-Labs#17.
The shape an array indexed by an integer array is the product of the array and index shapes. If both are large, this can produce a very large resulting array which makes the test run too slow (hypothesis deadline exceeded) and use too much memory.
…nstructors This also fixes ndindex(boolean_scalar) to correctly give a BooleanArray object instead of an Integer object.
Technically for integer scalars ndindex() will return an Integer(), rather than IntegerArray(), so it could vary in some tests if this is tested of not. However, the behavior should be identical for Integer, IntegerArray(integer scalar), and IntegerArray(shape () integer array) in all cases.
This adds a new helper function operator_index() that works like operator.index() except it disallows booleans. The built-in bool works with operator.index, but np.bool_ gives a deprecation warning, so according to our policy of making errors out of things that give deprecation warnings in NumPy, we make this give an error. This affects Integer(), Slice(), and asshape(). A slight compatibility break here against NumPy is that NumPy actually does allow booleans in slices. However, I'm not too worried about this as it's poor form to do that, and most likely would indicate a bug in user code.
…hat don't return an index Not only is this cleaner, as we don't have to "workaround" the function to pretend that it is testing an index, it fixes an issue where the previous way was not actually testing if a function raised an exception properly, because it would just be indexed anyway after calling the function. This affected newshape() and isempty() tests.
If an array index is created from another array index, this should be done. This is especially useful when creating a new array index out after broadcasting an existing array index, since broadcasting creates a memory efficient readonly view.
So far we disallow any other indices in the tuple. This also makes it so that Tuple.expand() broadcasts any arrays together.
Any slices, ellipses, or newaxes must not be between the arrays, or it raises NotImplementedError. This will probably never be implemented, because it's a bit of a wart case in the indexing API, which should raise an error (or at least that's what Travis told me to do). Also makes Tuple.expand() convert Integers into IntegerArrays as arrays when the tuple also contains arrays (so that they will also be broadcast).
This adds a private _axis keyword argument to newshape() methods. This is needed to keep the NumPy 1.19 behavior of not raising IndexError on out of bounds indices on empty arrays in some cases. This is needed because the specific cases when this happens require knowing the entire shape, but previously we just passed through idx.newshape(shape[i]) in Tuple.newshape(). This behavior will be deprecated in NumPy 1.20, at which point we will stop supporting it (breaking support earlier is hard because we cannot explicitly test against NumPy without a deprecation warning to catch). When this happens, the _axis argument to the newshape() methods will be removed and we can go back to just passing through shape[i] as before.
Now that Tuple.newshape() calls expand() instead of reduce(), it does not need to handle ellipses.
The test_ndindex_expand_hypothesis() already tests tuples because the ndindices strategy generates tuples, and it already tests all the same things that the test_tuple_expand_hypothesis test tests.
This removes duplication for the newshape tests. The integer and slice exhaustive tests are still in the respective files for the types.
This is needed because there are some changes in 1.20 that will make testing a lot easier. Right now we have to emulate some broken behavior that gives deprecation warnings in 1.20. Without the deprecation warnings, it is a lot harder to test because we can't catch the warning.
So far only the Tuple constructor an expand() are implemented properly.
There are some weird semantics with them where they sort of act like shape () arrays and sort of don't. It isn't that important to support right now, so we'll just leave it unimplemented.
The logic isn't correct in the case where boolean scalars are mixed with array indices, but this is currently disabled in the Tuple constructor.
See issue Quansight-Labs#77. This implementation of checking if the array shapes are subsequences of the test array shape doesn't seem to be fine grained enough for it to find a simple bug, so I think it will need to be improved.
That way we can accurately make a target value any time a boolean array doesn't match, as well as any time it does. This doesn't actually work well yet, but I think it's at least better than what I was doing before.
The hypothesis tests should catch this bug, but so far I haven't been able to make them without using explicit @examples.
|
I think this approach doesn't really work. A better way is to make smarter strategies that generate boolean arrays that are more likely to match the test array shape, which I've done in #74. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should not be merged.
This has a lot of commits from #74 which should go away when it is merged.
See issue #77. I've intentionally added a bug here to see if I can get hypothesis to find it. So far, I've been unsuccessful, even with many examples.