Added the code comment so I could add the PR comment. Is there a reason we are not using memmove here?

There has got to be a better way than writing my own 2-bit converter, but oh well...

I think PyArray_SizeConverter might be useful, but it probably belongs in a separate MAINT PR, since it has applications elsewhere. I think there's already a function you can call on a PyObject * to convert to an npy_intp, which would be better to just use in the scope of this PR.

Thanks. Could you point me to that function? I wasn't able to find something that matched. PyArray_intpConverter seems to do something entirely different.

BTW, I've moved this out of WIP status since all the tests passed in my local environment.

Just use PyArray_PyIntAsIntp, which takes a PyObject * and returns a npy_intp. A converter might be nice, but it's not in scope for this PR.

Why is it out of scope? I would end up writing the same bit of code anyway, and stashing it where it wouldn't be accessible...

If PyArray_SizeConverter is useful, it's useful with or without this patch. Conversely, this patch is implementable with or without that converter. There's no reason to bundle the changes together into one review.

My main problem with PyArray_PyIntAsIntp is that it does not allow None. What if I defined the converter locally, without adding it to any public APIs, and later submitted a PR for it if someone else wanted it?

Making it a public API would certainly make more sense in a separate review.

Also, you could just use:

npy_intp count = 0;
if (count_obj != Py_None) {
    count = PyArray_PyIntAsIntp(count_obj);
    if (error_converting(count)) {
        goto fail;
    }
}

I think I like the idea of a similar converter, but I'd like to see it:

• In a separate PR
• With examples elsewhere in numpy showing it is more widely useful

We don't want to add something to the public API without being sure we chose the right definition. As you observe, PyArray_intpConverter doesn't do what you hoped it would.

Gotcha. Sorry for the slowness.

Please let me know if something like this is defined elsewhere...

I'll be totally honest, I have no idea what I did here or why. The error messages made it pretty clear that the indices need to be unique, but I have no idea why or what they do. Should I have just set this one to 304 (the next available number)? If so, why aren't they all just being generated?

These numbers are not allowed to change, because that will break ABI compatibility for code compiled against previous versions of numpy - so your new number must go at the end,

Definitely will fix then.

What do the numbers actually do?

They are indices in a table of function pointers, which are used to export the functions to other python extension modules.

This bit really needs some review. Is there a reason memcpy wasn't used for this before? If not, did I use the correct sizeof?

Is there any reason that memcpy should be used here? It's less safe, and the loop should optimize to it anyway.

I was just messing around. I'll go ahead and revert if you think that is better.

If the type of outdims is wrong, this does the wrong thing in all cases, where the previous code would warn and only fail in overflow cases.

Since it's not part of this new feature, and not a clear improvement to me, I'd prefer it not be part of an ENH PR.

The goal was to see if I could get rid of i.

* 8 would be much clearer here

Strangely enough, I decided to keep the existing convention for this bit. I rather prefer << 3 for some reason.

Why did this move? Better to leave things in the narrowest scope possible

Yes. Forgot to move it back when I realized I could optimize the stride=1 loop without using the mask. It's going back ASAP.

nit: space after for

Not sure the negative number is a useful feature here

Padding with zeros would be more useful here, I think?

@seberg. As you can see here, myarray was never an option to begin with. The documented keyword name never actually what the function was parsing, and the name being parsed was an illegal name. Although I suppose you could do

kw = {'in': ...}
unpackbits(**kw)

Fair enough. a is better anyway. I wonder if we should at some point work on unifying most of these names (basically a backward incompatible change that would in practice probably hit almost no-one).

Single quotes are a shortcut for :py:value and are meant to be linked to python documentation. When creating the link fails, the text is rendered in italics. Since the intention here is for code formatting, they should all be two quotes. You can see the difference in the way the default axis argument None used to be rendered here vs this PR here

@mattip. Most of the docs I see have single quotes. I prefer the aesthetic of the italicized version to the code formatted one, and technically None is a python object. However, I don't care much one way or the other, so will fix as you request ASAP.

It is strange that with single quotes, it wouldn't link to the python documentation of None already. Indeed, that seems a bug...

I think I slightly prefer the double backticks for None (there is no point in linking it to the python docs anyway).

https://docs.scipy.org/doc/numpy/docs/howto_document.html has some examples using single ticks for the kwargs, and actually nothing for None. Could be nice to just decide for once, not sure I care though.

I'll grep and count both versions in the existing code. My intuition tells me that single ticks dominate, but I'm not sure. Majority rules? :)

None is just an example here. There are other single-ticked words in the documentation change: a, count, index,

Yeah, although for a, count and axis at least, they are arguments, and the howto document examples use single backticks for those. However, while I would guess a lot of people somewhat use that document, I doubt it is applied consistently with respect to these examples in any case.

Let's call it a niggling nit and ignore it, is this ready for merge otherwise?

There were 145 occurrences of None with single quotes and 147 occurrences with double quotes until this PR came along. I will change back to double. Sorry for the holdup.

@mattip If you want me to go through and normalize all the other occurences, I can do that fairly quickly, since I have the full list and a script that can do it right in front of me.