BUG: Create complex scalars from real and imaginary parts - #23780
Conversation
| if (PyArg_ParseTupleAndKeywords(args, kwds, "|OO", kwnames, &real_obj, &imag_obj)) { | ||
| if (imag_obj == NULL) { | ||
| obj = real_obj; | ||
| Py_XINCREF(obj); |
There was a problem hiding this comment.
The refcounting for this is off below, hard to fix without a goto finish or so probably. Might be easier to just include the scalar creation/assignment in the branch directly (duplicating some code)...
There was a problem hiding this comment.
Can you explain a bit more what the refcounting issue is? I think with the latest push everything is correct.
We INCREF obj if it gets assigned to real_obj, but then a decref happens after obj gets used in PyArray_FromAny. There's an early return before that in the obj == NULL case, but then in that case we can't have any refcounting errors because obj is NULL.
There was a refcounting issue in the original version of this PR if getting the typecode failed, but as you pointed out in another comment, that's impossible given the way it's written. I deleted the error case and left a comment that it can't fail to make that clearer.
There was a problem hiding this comment.
Ah, yes you are right, I thought there were more error paths, but there is mainly that obj == NULL block.
| Py_XINCREF(obj); | ||
| } | ||
| else if (PyNumber_Check(real_obj) && PyNumber_Check(imag_obj)) { | ||
| if (PyComplex_Check(real_obj) || PyComplex_Check(imag_obj)) { |
There was a problem hiding this comment.
OK, without this we would allow np.complex128(numpy_complex, numpy_complex), although it would give a warning. I don't have a strong opinion on the need, I agree, this is mainly about accepting the trivial case of np.complex128(3., 4) with python scalars anyway.
| obj = PyComplex_Type.tp_new(&PyComplex_Type, args, NULL); | ||
| if (obj == NULL) { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
While annoying, duplicating the scalar creation may be easier here? Something like this (but I am not sure the cast is actually valid, may have to cast real/imag separately.
npy_@name@ val = PyComplex_AsCComplex(obj); // does that work, since its struct-like
// Need to fetch the typecode. Note that this can't actually fail
// (unlike the code below makes you believe).
PyObject *robj = PyArray_Scalar(&val, typecode, NULL);
Py_DECREF(py_complex);
return robj;
There was a problem hiding this comment.
Unfortunately Numpy at the moment allows any array-like to be passed to the scalar initializers (and there are tests for it), which means I'd first need to re-implement the array-like coercion logic in PyArray_FromAny to get this to work.
In [2]: np.complex128([1, 2, 3, 4])
Out[2]: array([1.+0.j, 2.+0.j, 3.+0.j, 4.+0.j])
In [3]: np.complex128(b"1+4j")
Out[3]: (1+4j)
I think it's better to just avoid the early return and re-use the logic in the rest of this function that goes through PyArray_FromAny.
d3ff38c to
6d3bed8
Compare
|
I will put this in, thanks @ngoldbaum. I will not that I think this chunk of code needs a heavy-handed rewrite while being open to just break some stuff and keeping these fallbacks probably only where it's too difficult to decide how to just delete (i.e. possibly strings). |
2 participants
fixes gh-19125
If anyone has an idea for how to generalize this for
np.clongdoubleI'm all ears. I couldn't come up with a straightforward way to do that. I can't go through python complex numbers forclongdoublebecause the cast fromclongdoubletocomplexis unsafe. Proposing this PR as-is without that support because extended precision types likeclongdoubleare already kind of an odd duck.