Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: changing the complex struct has implications for Cython #26029

Closed
mattip opened this issue Mar 14, 2024 · 7 comments · Fixed by #26080
Closed

BUG: changing the complex struct has implications for Cython #26029

mattip opened this issue Mar 14, 2024 · 7 comments · Fixed by #26080
Milestone

Comments

@mattip
Copy link
Member

mattip commented Mar 14, 2024

Cython turns the property access increment-in-place in something like

def inc1_cfloat_struct(np.ndarray[np.cfloat_t] arr):
    arr[1].real += 1
    arr[1].imag += 1

into this C code (cleaned up and simplified a bit from the actual code)

(* (npy_cfloat *)__pyx_pybuffernd_arr.rcbuffer->pybuffer.buf + 1)).imag += 1;

but npy_cfloat on NumPy2.0 no longer is a struct with real and imag fields. See cython/cython#6076 (comment). Can we give easy left-hand access to the imag part of a complex type? The migration guide mentions npy_csetreal and npy_csetimag, but then the codegen will need to do something very different.

cc @lysnikolaou

This can be hit by running python runtests.py numpy_test after applying the changes in cython/cython#6076

@da-woods
Copy link

Just to clarify a bit:

The Cython wrappers for Numpy describe the structure as:

ctypedef struct npy_cfloat:
float real
float imag

which doesn't match any more.

One of the places that it affects is in the Cython tests. We can change that fine because it's something we control. The bigger problem is if other people are using it via this interface.

@lysnikolaou
Copy link
Contributor

Can we give easy left-hand access to the imag part of a complex type?

One could do ((float *)__pyx_pybuffernd_arr.rcbuffer->pybuffer.buf + 1)[0] += 1; for the real part and ((float *)__pyx_pybuffernd_arr.rcbuffer->pybuffer.buf + 1)[1] += 1; for the imaginary part, which is basically what npy_csetreal/imag does.

The Cython wrappers for Numpy describe the structure as [...] which doesn't match any more. [...] The bigger problem is if other people are using it via this interface.

Could you describe the problem in a bit more detail? I'm not very familiar with how Cython does things here, and while I understand conceptually that having a different definition in C and in Cython is not ideal, I don't get what exact problem users would hit.

@mattip
Copy link
Member Author

mattip commented Mar 17, 2024

Could you describe the problem in a bit more detail?

TL;DR: code that used to access the imag and real fields will fail to compile on NumPy2. Also: the __init__.pxd and __init_.cython-30.pxd files in numpy.2.0.0b1 have incorrect declarations of npy_cfloat, npy_cdouble, npy_clongdouble, and others that cause a compilation error that is difficult to decode.

Longer version:

There is a usage pattern in cython, which is reflected in a test, that looks like

def inc1_cfloat_struct(np.ndarray[np.cfloat_t] arr):
    arr[1].real += 1
    arr[1].imag += 1

This test and usage pattern fails on NumPy2 since the complex dtypes are no longer a struct with the imag and real fields: they are now some opaque compiler-defined type.

The recommended way to handle complex dtypes is via npy_creal, npy_cimag, npy_csetrealandnpy_csetimag`.

So there are two problems:

  • Usage patterns that have worked in the past will have to change, breaking existing user code
  • There doesn't seem to be a way to write cython code that will work on both NumPy1 and NumPy2: the macros npy_creal and friends are not exposed in __init__.pxd. Even if we add them for NumPy2, what do we do about NumPy1?

@seberg
Copy link
Member

seberg commented Mar 18, 2024

what do we do about NumPy1?

Hmmmm, annoying, we would have to ask users to copy the definition of npy_real for compiling against NumPy 1?
That could be a small numpy_complex.pyd file to copy in, but not sure how many libraries will be affected.

Is there any way we can keep exporting the struct definition for Cython (or some other flag)!?

@lysnikolaou
Copy link
Contributor

What we did on SciPy was something along the lines of:

cdef extern from "npy_2_complexcompat.h":
    void NPY_CSETREAL(np.npy_cdouble *c, double real) nogil
    void NPY_CSETIMAG(np.npy_cdouble *c, double imag) nogil

cdef inline double_complex zpack(double zr, double zi) noexcept nogil:
    cdef np.npy_cdouble z
    NPY_CSETREAL(&z, zr)
    NPY_CSETIMAG(&z, zi)
    return double_complex_from_npy_cdouble(z)

This should work with both NumPy 1 and NumPy 2. Here's the relevant piece of code.

@mattip mattip added this to the 2.0.0 release milestone Mar 18, 2024
@mattip
Copy link
Member Author

mattip commented Mar 18, 2024

I marked this as a blocker for 2.0.0rc1. The pxd files we ship with NumPy need fixing: at a minimum they should expose npy_setreal and friends.

@seberg
Copy link
Member

seberg commented Mar 18, 2024

What if we just define the complex types to:

ctypedef float complex npy_cfloat
ctypedef double complex npy_cdouble
ctypedef long double complex npy_clongdouble

ctypedef float complex npy_complex64
ctypedef double complex npy_complex128

(without any from extern). Doesn't that define it to the identical thing on NumPy 2, and Cython takes care of defining .imag and .real? Or is there some weird system thing that that actually ends up at types which are not compatible to the compiler?

seberg added a commit that referenced this issue Mar 20, 2024
Fixes #26029

declare npy_cfloat and friends as empty structs
declare cfloat_t and friends as ctypedef float complex cfloat_t, without being directly connected to npy_cfloat. This allows still using .imag and .real attributes, which Cython wraps with macros
add a test for BUG: changing the complex struct has implications for Cython #26029, it passes since we use C compilation for cython. The inplace += will fail on C++.
resync the two cython pxd files. At what point do we declare NumPy requires Cython3?
I checked that scipy and cython can use the pxd file.

---------

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
charris pushed a commit to charris/numpy that referenced this issue Mar 22, 2024
Fixes numpy#26029

declare npy_cfloat and friends as empty structs
declare cfloat_t and friends as ctypedef float complex cfloat_t, without being directly connected to npy_cfloat. This allows still using .imag and .real attributes, which Cython wraps with macros
add a test for BUG: changing the complex struct has implications for Cython numpy#26029, it passes since we use C compilation for cython. The inplace += will fail on C++.
resync the two cython pxd files. At what point do we declare NumPy requires Cython3?
I checked that scipy and cython can use the pxd file.

---------

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants