-
Notifications
You must be signed in to change notification settings - Fork 280
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
ENH: Add a few bitarray functions and tests #4525
Conversation
Currently, one of the tests does not pass! Hooray!
|
If this passes, could I get a review on it? |
@yt-fido test this please |
Hmm, this doesn't look like my error, but maybe it is? Did I miss marking something as C++?
|
@matthewturk not your error, that's the flaky build error that happens on the full testsuite frequently. I'm actually not sure why it happens... |
@yt-fido test this please |
I'm not sure either, though I suspect a symptom of race condition in the parallel build (possibly related: #4611). It's been happening since we switched jenkins from Python 3.8 to 3.10 |
@@ -14,7 +14,21 @@ cimport numpy as np | |||
|
|||
cdef inline void ba_set_value(np.uint8_t *buf, np.uint64_t ind, | |||
np.uint8_t val) noexcept nogil: | |||
# This assumes 8 bit buffer | |||
# This assumes 8 bit buffer. If value is greater than zero (thus allowing |
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.
Just wanna say I very much appreciate this detailed comment.
@@ -26,6 +26,7 @@ def test_inout_bitarray(): | |||
b = ba.bitarray(arr=arr_in) | |||
arr_out = b.as_bool_array() | |||
assert_equal(arr_in, arr_out) | |||
assert_equal(b.count(), arr_in.sum()) | |||
|
|||
# Let's check we can do something interesting. | |||
arr_in1 = np.random.random(32**3) > 0.5 |
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.
you haven't touched these lines, but it looks like another spot where we missed replacing np.random.random
with the newer syntax (see #4733). if you want to update this whole function while you're here you'd want to put a
rng = np.random.default_rng()
up above then replace all the np.random.random
calls here with rng.random
. We can also do this in a separate PR.
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 made the change -- thanks!
self-merging because the tests pass and I want to get some stuff done before Chris leaves for the day (and this will make that easier) |
In prep for some other work, here are additional items for bitarrays that make them nicer to use. Tests are added, and I think I got the edge cases.