TST: add tests for take() on empty arrays by jorisvandenbossche · Pull Request #20582 · pandas-dev/pandas (original) (raw)

Putting @jreback's comment here to not have it lost in a hidden inline comment:

I already responsded. take_1d can certainly be used in an implementation of this. This is the general implementation of virtually all take behavior, so naturally a check for no indexers should happen there, rather than in each higher level implementation (e.g. EA). The point is that the way you are advocating is simply more complex / buggy and adds more code. Virtually all EA methods should use API's that pandas already has. Should these be public. Sure if possible. But for something like this we don't want to make the actual implementation public as it has certain guarantees. However I don't see any problem with an internal implementation (like EA) using it. it fact I would say they have / should use it. otherwise how else would categorical/DTI/II be implemented at all?

If you can point me to an answer that is not just "this should be in take_1d", I don't find it :)

I removed the changes in this PR regarding Categorical (and opened a separate issue for that: #20664), so only adding tests for the ExtensionArray and the test implementations.
So we can keep bigger take_1d changes for another PR.

take_1d is internally used in many cases where the indexer passed to take_1d is the result of eg get_indexer, so this ensures that the indexer is correct, and for those no out-of-bounds checks are needed.

You advocate for that ExtensionArrays should use pandas take implementation, but at the same time say that you don't want to make it public? That seems to contradict each other.

If we want to expose our take implementation to extension array authors (and maybe we are that ourselves as well), I would personally create a new take functions which just calls the existing take_1d, but additionally does first bounds checking.
That way, we keep on using the faster take_1d functions internally where we know bounds checks are not needed, and we can make the new take method public for external usage as well.