ENH: Throw TypeError on operator concat on Numpy Arrays #16570

vrakesh · 2020-06-10T23:32:55Z

New to Python C-API, requesting feedback on Reference counter handling, does it look correct?

Tried to reuse Numpy concatenate.

vrakesh · 2020-06-11T00:24:25Z

Follow up question, should we allow concat with regular arrays like np.concatenate?

operator.concat does not allow concat of two types . Should we keep the numpy way or follow operator.concat in this specific instance?

numpy/core/src/multiarray/sequence.c

numpy/core/tests/test_shape_base.py

numpy/core/src/multiarray/sequence.c

eric-wieser · 2020-06-11T07:12:51Z

PyPy failure is either a (possibly unfixable) bug in PyPy, or a design problem in python itself.

vrakesh · 2020-06-11T18:58:48Z

@eric-wieser Thanks a lot for the feedback, made the changes. Quick question, should we support coercion of array like types to NDarray or follow operator.concat for this case?

mattip · 2020-06-11T19:34:40Z

The test shows an inconsistancy in Python. operator.concat is documented as "Return a + b for a and b sequences." PyPy will prefer nb_add, and only if it is NULL try sq_concat. CPython seems to do something different, This PR changes the existing results for operator.concatenate and in my opinion is incorrect. Could you provide the motivation for the change?

seberg · 2020-06-11T19:40:02Z

@mattip the C-Python implementation is roughly this:

if sq_concat is not NULL:
    return sq_concat()

if isinstance(obj1, sequence) and isinstance(obj2, sequence):
    return obj1 + obj2

I think this is correct at least in the sense that the current behaviour is undesired enough that it makes sense to do something. I would be happy, however, to implement this simply as:

def sq_concat(self, other):
    raise TypeError("NumPy arrays cannot be concatenated using `operator.concat` use `np.concatenate` instead.")

if this does not work in PyPy. I agree that this is all besides the standard, but currently operator.concat(a, b) returns a + b, which is also not great?

mattip · 2020-06-11T19:54:14Z

According to the documentation, a+b is exactly what operator.concat should be returning, no? Could you point to documentation of operator.concat as your pseudo-code? I don't debate that is what CPython does do, just that is not what the Python documentation says operator.concat should do.

mattip · 2020-06-11T20:06:37Z

The CPython implementation of operator.concat calls PySequence_Concat, which is documented as

Return the concatenation of o1 and o2 on success, and NULL on failure. This is the equivalent of the Python expression o1 + o2.

Inside PySequence_Concat is this comment

       /* Instances of user classes defining an __add__() method only
       have an nb_add slot, not an sq_concat slot.      So we fall back
       to nb_add if both arguments appear to be sequences. */

so it seems a situation where both nb_add and sq_concat are set is not allowed. I am interested why ~~@vrakesh proposed this PR~~ @eric-wieser opened this issue: is there a use-case for changing the semantics of operator.concat on ndarrays?

Edit: @eric-wieser opened the issue.

seberg · 2020-06-11T20:08:09Z

Well, I agree with both sides. Yes, my pseudo-code is what it does, and it implements it as a+b for sequences... Now we have the problem that numpy arrays are almost, but not really, sequences.

So, I see the argument that we should maybe not try to do "magic" to make this work. But on the other hand, I am not sure there is a reason why not to raise an error? Since it is guaranteed that any user who does operator.concat(arr, arr) is not getting what they expect.

@eric-wieser noted the possibility when torch.concat came up as a different spelling for concatenate. And to me also it seemed strange, but OK to define on first sight (i.e. the current behaviour is just bad, so even something a bit hackish seemed OK to me).

anirudh2290 · 2020-06-11T20:39:00Z

giving it some thought, @mattip makes a good point, looks like it should call nb_add, which in the case of python, which when we pass numpy arrays means broadcast and do an elemwise add.

but yes very few users would expect that operator.concat would cause a broadcast followed by elemwise add and an error will help. not sure, if we go down this road though, how many more such difference in behaviors (because of numpy array being slightly different from python seq) we will have to fix.

eric-wieser · 2020-06-11T20:48:17Z

Perhaps we should file a python issue asking for clarification.

eric-wieser · 2020-06-11T21:21:03Z

In https://bugs.python.org/issue29139#msg284547, @serhiy-storchaka says:

Third-party classes (maybe NumPy arrays, I don't know) can have different implementations of sq_concat and nb_add.

seberg · 2020-06-11T21:49:44Z

Honestly, I am unwilling to fight either way. If Python really embraced it, they would need to add __concat__, until then this is all feels like an implementation detail.
I would be happy to e.g. raise an error, just because I do not see it hurting anyone, but even the raised error would be an implementation detail itself (since PyPy may behave differently).

eric-wieser · 2020-06-12T11:43:25Z

Just attempted to post to python-dev about this, ~~but it looks like my email got swallowed by the mailing list... Maybe~~ the archives are just slow.

https://mail.python.org/archives/list/python-dev@python.org/message/HCXMI7LPWZEKKPRIJTVHN5ZRWIUXM5C3/

vrakesh · 2020-06-12T17:16:26Z

Thanks @eric-wieser for the email, hope the clarification helps us move this in the right direction.

seberg · 2020-06-16T19:14:22Z

Seems like the (small) discussion on the python mailing list basically converged to: Its an implementation detail, but putting an error could be fine.
I will let @mattip make the call on what he prefers. The best I can offer would be make it a TypeError, with text like:

Calling sequence concatenation (operator.concat) on NumPy arrays invokes undefined behaviour. Please use np.concatenate instead. This error is an implementation detail and may not be raised on all Python versions/implementations, in which case incorrect results may be returned.

EDIT: We would not really be able to test it, or at least skip it on PyPy and any other python implementation which behaves differently

vrakesh · 2020-06-22T20:03:20Z

@seberg @mattip Does this look good? Am I missing any specific changes related to PyPy?

rossbar · 2020-06-22T20:11:39Z

numpy/core/src/multiarray/sequence.c

+    // Throw a type error, when trying to concat NDArrays
+    if(PyArray_Check(other) || PyArray_Check(self)) {
+        PyErr_SetString(PyExc_TypeError,
+                        "Concatenation operation is not implemented for Numpy Arrays");


Maybe it's worth highlighting np.concatenate in the error message? Only as long as it doesn't make things too verbose.

Agreed, makes sense to redirect

seberg · 2020-06-22T20:11:45Z

You have to skip the test on PyPy (there is an IS_PYPY variable). Also PyArray_Check(self) will always be true, so you don't even have to put it.

vrakesh · 2020-06-22T20:37:56Z

Thank you @seberg will make the changes.

Note that this is a Python implementation detail and *not* a language standard. Users should not rely on the error being set, as it may not be set e.g. on PyPy. (See numpygh-16570 for discussion, this has been briefly discussed on Python as well: https://mail.python.org/archives/list/python-dev@python.org/message/HCXMI7LPWZEKKPRIJTVHN5ZRWIUXM5C3/ Fixes numpygh-16489

seberg · 2020-06-27T16:25:22Z

Thanks Rakesh!

vrakesh requested review from seberg and eric-wieser June 10, 2020 23:33

vrakesh changed the title ~~WIP:Add support sq_concat on ndarrays~~ WIP:Add support for sq_concat on ndarrays Jun 11, 2020

vrakesh force-pushed the concat_sequence branch from 2cb9dac to 5e498a5 Compare June 11, 2020 00:19