There probably is a more elegant way to flip the bits

I think you need to rewrite the loop about, and not do build <<= 1, but rather OR-in an already shifted element (i.e., something like build |= (inputr[j] != 0) << shift where shift depends on order and j).

adopted

You mention "adopted" but I think the code is still the same, no? I meant the loop above (not "about"), which now has the build <<= 1. I think that loop can be rewritten to just do the right thing for each order.

I could write this as below, is that better? I don't understand why I needed to use unsigned char instead of char for the order != 'b' case, I am only ever shifting right. If unsigned char is required it should be for the left shifts, no?

    for (; index < n_out; index++) {
        unsigned char build = 0;
        int i, maxi;
        npy_intp j;

        maxi = (index == n_out - 1) ? remain : 8;
        if (order == 'b') {
            for (i = 0; i < maxi; i++) {
                build <<= 1;
                for (j = 0; j < element_size; j++) {
                    build |= (inptr[j] != 0);
                }
                inptr += in_stride;
            }
            if (index == n_out - 1) {
                build <<= 8 - remain;
            }
        }
        else
        {
            for (i = 0; i < maxi; i++) {
                build >>= 1;
                for (j = 0; j < element_size; j++) {
                    build |= (inptr[j] != 0) ? 128 : 0;
                }
                inptr += in_stride;
            }
            if (index == n_out - 1) {
                build >>= 8 - remain;
            }
        }
        *outptr = (char)build;
        outptr += out_stride;
    }

I think this was a bug, if not we need to add it back in. I don't have a big-endian system to test on.

Hmm, this does get turned into bytes in the end, and those will have a different order in the uint64 here. So, my guess is that you do need to swap (i.e., have the reverse swap for big and little endian). But am a bit too lazy to just write it out... Note that the test cases surely would fail for big endian if this was wrong...

Now tested on a big-endian system since I have access to the gcc build farm and a sparc64 machine. See 5d1b9208d

Flip around for packbits [0, 0, 0, 0, 0, 1, 1] => 3

fixed

I think you need to rewrite the loop about, and not do build <<= 1, but rather OR-in an already shifted element (i.e., something like build |= (inputr[j] != 0) << shift where shift depends on order and j).

Put this in an else clause of the if statement below.

fixed

I'm confused: exactly the same v is constructed here as below, the only difference being the byte swap. Can we just fill both big and little in one loop? (one the unswapped, the other the swapped v)?

fixed

Hmm, this does get turned into bytes in the end, and those will have a different order in the uint64 here. So, my guess is that you do need to swap (i.e., have the reverse swap for big and little endian). But am a bit too lazy to just write it out... Note that the test cases surely would fail for big endian if this was wrong...

Adjust the comment to make clear this is for the "big-endian" case.

removed reference to endian, since the 'b' makes the intention clear. Moved the rest of the comment to mark the intrinsic

You mention "adopted" but I think the code is still the same, no? I meant the loop above (not "about"), which now has the build <<= 1. I think that loop can be rewritten to just do the right thing for each order.

Good to adjust this comment too...

"fixed" by removing "big endian"

Can we match python here in int.from_bytes (C longobject.c:int_to_bytes_impl), and actually require the full string?

Could consider using the internal _PyUnicode_EqualToASCIIId, which I think is available in the python versions we care about.

adopted

I used strncmp instead, which does not require Py_LIMITED_API

We already depend on Py_LIMITED_API, so I don't think we need to care

one of the test runners failed when I used _PyUnicode_EqualToASCIIString, saying the function was not defined.

Edit: fix function name

There's no guarantee that char(256) overflows, CHAR_BIT might not be 8.

Why not just use a simple loop here?

for (int i = 0; i < 8; i++) {
    *outptr = (bool)(*inptr & (1 << i));
    outptr += out_stride;
}

adopted

Shouldn't there be 8 items in this list, not 7?

I'm not sure I'd call this the common standard - I frankly find it bizarre that unpackbits(1)[0] does not refer to "bit 0" of x.

Could instead compare to binary literal syntax, stating that 0b00000011 => [0, 0, 0, 0, 0, 0, 1, 1]

Seeing the comment, I remember being bitten by exactly this (which is of course why "little" will be useful!).

Maybe the docstring can be agnostic about which is "standard" - both have advantages. I like the idea of adding 3 = 0b00000011 => [...]

adopted

The order refers to the list of bits in the input.

Rename to bitorder?

redid the comment and changed order -> bitorder

Should be

    bitorder : {'big', 'little'}, optional

Also below.

fixed

Periods, Matti, periods.

added

Period. But I will fix it when editing the release note.

how does the removal of the lookup table impact performance?
As I recall it was significant when I added it.