Implement BooleanArray #71
Merged
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
This will be the common superclass of IntegerArray and BooleanArray.
c.f. issue Quansight-Labs#17.
So far no methods are defined on it.
This also implements BooleanArray.count_nonzero, which returns the number of True elements of the index.
The array tests can be slower than the default 200 ms if they receive a very large list input that has to be converted into an array.
NumPy has a fast path that incorrectly compares the sizes of the array and boolean index, rather than the shapes. As a result, invalid indices can be accepted in some cases when the size but not the shapes match, and in other cases it gives the wrong exception. This is being fixed in the upstream NumPy (see the issue numbers linked in the comments). Rather than matching the broken NumPy behavior here, we do the right thing, and guard against the broken NumPy behavior in the tests.
They can all just use newshape() if the shape is given.
|
This is ready for review. I'm going to implement the remaining bits in a new PR. |
- Fix the documentation of the dtype attribute - Include the inherited methods from the ArrayIndex superclass - Add ArrayIndex to the internal API docs - Fix some formatting issues in some docstrings
scopatz
reviewed
Aug 5, 2020
| @@ -24,6 +24,10 @@ | |||
|
|
|||
| __all__ += ['IntegerArray'] | |||
|
|
|||
| from .booleanarray import BooleanArray | |||
|
|
|||
| __all__ += ['BooleanArray'] | |||
There was a problem hiding this comment.
It is odd to build up __all__ programatically like this. Seems like there should be a single definition of it
There was a problem hiding this comment.
I think it's a common way to do it. It keeps all the __all__ items next to their imports.
There was a problem hiding this comment.
Hmm weird... I don't think I have ever seen it done this way
scopatz
reviewed
Aug 5, 2020
| @@ -0,0 +1,122 @@ | |||
| import warnings | |||
There was a problem hiding this comment.
Please add a docstring to this module
scopatz
reviewed
Aug 5, 2020
scopatz
reviewed
Aug 5, 2020
| @@ -0,0 +1,149 @@ | |||
| from numpy import bool_, count_nonzero | |||
There was a problem hiding this comment.
Please add module level docstring
scopatz
reviewed
Aug 5, 2020
|
Generally looks really good @asmeurer - just a few small changes / questions |
It doesn't completely work, but it should hopefully work in the future.
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 also adds a document on type confusion (see #17).