BUG: Compiling C extension against numpy 2.0.1 fails with error: 'I' undeclared

Describe the issue:

Attempting to build the gstlal-inspiral package (C modules, C extension modules, and pure Python modules built using autotools) fails in compiling a C extension module, in an environment with numpy 2.0.1 with:

/home/conda/feedstock_root/build_artifacts/gstlal-inspiral_1722339059186/work/python/_spawaveform.c: In function 'SPAWaveformReduceSpin':
/home/conda/feedstock_root/build_artifacts/gstlal-inspiral_1722339059186/work/python/_spawaveform.c:585:55: error: 'I' undeclared (first use in this function)
  585 |         hOfF[k] = amp * (cos(Psi+shft*f+phi0+piBy4) - I*sin(Psi+shft*f+phi0+piBy4));
      |

The I in question is supposed to be that provided by complex.h.

This was briefly discussed here (and replies).

Reproduce the code example:

n/a

The source code for the extension module in question is here.

Error message:

/home/conda/feedstock_root/build_artifacts/gstlal-inspiral_1722339059186/work/python/_spawaveform.c: In function 'SPAWaveformReduceSpin':
/home/conda/feedstock_root/build_artifacts/gstlal-inspiral_1722339059186/work/python/_spawaveform.c:585:55: error: 'I' undeclared (first use in this function)
  585 |         hOfF[k] = amp * (cos(Psi+shft*f+phi0+piBy4) - I*sin(Psi+shft*f+phi0+piBy4));
      |

Python and NumPy Versions:

Python 3.10.4
Numpy 2.0.1

Runtime Environment:

No response

Context for the issue:

This briefly prevented forward motion on conda-forge/gstlal-inspiral-feedstock#24 (until the addition of a patch) (that build is eventually foiled by other issues, but the fundamental problem here stands independently, I think).

From #26789:

No, if people notice, we may want to add docs on how to re-add I, I guess. But I guess we hope that nobody really notices anyway.

Somebody noticed .... no way to win here really.

Given that there is no good solution, I think it is better to not undefine macros that are part of the C standard. Anyone working in C has to deal with the transitive include issue, and once they start including headers from a library they don't control, they should be prepared to avoid names that come from any header in the standard library.

For discussion, here is the fix. Maybe we should add this to the documentation. Our choices are either get cython (and maybe others?) to undefine I after including numpy/arrayobject.h or have numpy-c-api consumers fixing complex code like this.

#include <numpy/arrayobject.h>

/* re-add `I` after numpy removed it */
#ifndef I
#define I _Complex_I
#endif

@mattip

@mattip I am also working with a different project that is seeing the same issue via cython. Their code is here, so if there's an equivalent patch for cython-based projects that attempt something like below, I'll be glad to test it.

...
cimport numpy as np
cdef extern from "<complex.h>":
    double cabs(double complex z)
    double creal(double complex z)
    double complex cexp(double complex z)
    double carg(double complex z)
    double complex I
...

@moorepants

The same patch should work for this other project. The problem arises when, as @moorepants stated in the original issue #26787

This specifically results when users in SymPy have chosen that a symbolic variable be named Symbol('I') and then we print C code using that variable and compile on the fly with Cython in our various code generation tools.

Perhaps we should encourage them to change their workflow.

SymPy generates NumPy linked C code and if we had a definitive list of all reserved variable names, we can deal with them in the SymPy printers, e.g. change double I to double sympy_I. We already appropriately handle all reserved variables in the standard C library.

So perhaps we should revert #26789 going forward and encourage SymPy to wrap all variable names, like cython does, to avoid any possible clashes.

One hesitation I have in reverting is that the error when using double I after include "complex.h" is very opaque. The I undeclared error here is much easier to google for.

encourage SymPy to wrap all variable names

I'm not sure we can/should do such a thing. SymPy users rely on the ability to control the variables in generated C code and we've already seen some resistance to managing the variable clashes for the user. We may be able to do it in some specific use cases (like the ufuncify() example I gave), but doing something broader would require more careful attention.

Numba is also affected by this issue. Numba 0.60.0 fails to build from source:

In file included from numba/_helpermod.c:23:
  numba/_helperlib.c: In function ‘numba_cpowf’:
  numba/_helperlib.c:22:44: error: ‘I’ undeclared (first use in this function)
     22 |     #define _complex_float_ctor(r, i) (r + I * i)
        |                                            ^
  numba/_helperlib.c:147:12: note: in expansion of macro ‘_complex_float_ctor’
    147 |     *out = _complex_float_ctor((float) _out.real, (float) _out.imag);
        |            ^~~~~~~~~~~~~~~~~~~
  numba/_helperlib.c:22:44: note: each undeclared identifier is reported only once for each function it appears in
     22 |     #define _complex_float_ctor(r, i) (r + I * i)
        |                                            ^
  numba/_helperlib.c:147:12: note: in expansion of macro ‘_complex_float_ctor’
    147 |     *out = _complex_float_ctor((float) _out.real, (float) _out.imag);
        |            ^~~~~~~~~~~~~~~~~~~
  error: command '/usr/bin/gcc' failed with exit code 1

We discussed this at a recent Community meeting. There were voices for both approaches, but the discussion led to a prefererence to stay with the current solution (#undef I) and document it better. One of the arguments is that boost uses I in its public headers.

Okay, but it's going to introduce some pain for users. It's non-trivial to re-define I correctly. Your example does not work on Windows and is incorrect for POSIX-like OS. It's only correct for current versions of glibc on Linux.

Microsoft's C compiler needs a different approach than C compiler on a POSIX-like OS.

I is defined as the complex type _Fcomplex initialized by { 0.0f, 1.0f }.

The Open Group basedefs standard defines two ways for I on POSIX-like OS:

I
Expands to either _Imaginary_I or _Complex_I. If _Imaginary_I is not defined, I expands to _Complex_I.

This (untested) code should do the trick on Windows and POSIX-like OS:

#if defined(_MSC_VER)
  #define I _FCbuild(0.0f, 1.0f)
#elif defined(_Imaginary_I)
  #define I _Imaginary_I
#elif defined(_Complex_I)
  #define I _Complex_I
#else
  // non-POSIX, not MSVC
  #error "Don't know how to define 'I'"
#endif

@tiran

Thanks @tiran. I copy-pasted that into the PR.

Another idea might be to put all our complex-number header file pieces inside a #ifdef USE_NPY_COMPLEX stanza, so only users who opt-in to using the numpy complex number C-API will see the mess in the first place. Will that then work with code generators like Cython?

USE_NPY_COMPLEX is a clever idea! How about you make the undef of I conditional?

#if defined(I) && !defined(USE_NPY_COMPLEX)
  #undef I
#endif

@duncanmmacleod

I am a maintainer of the other library @duncanmmacleod mentioned. I believe in general it is not a good practice to modify the content of a /standard/ library (which is what the current #undef I does here): numpy does not control the downstream chain of includes - all assuming a non-modified standard library - .

@moorepants

From the SymPy point of view, I believe we can work around this downstream (at least for just the I).

Indeed, @moorepants as stated by this the recommandation is to check the existence of the define _Imaginary_I for complex support (and in that case #undef I if defined...).

@rgommers

So as far as my understanding was, the state right now is a bit messy:

It is unclear if undoing it again in 2.0.x isn't annoying, bug-fix releases should ideally not break compilation (again).
Although, for 2.1 that seems a bit less of a problem to me. Overall, I do not think this is a big enough problem that we shouldn't just do what seems best.
SciPy apparently had some failures with boost using I in some places and there was an argument that others might run into that as well. @rgommers seemed to think others projects are likely to run into that particular thing.
The error when using I incorrectly was supposedly pretty cryptic. For example for a variable declared to I, it may look like:
```
<source>:5:8: error: expected identifier or '(' before '__extension__'
 double I = 3;
```

The main question to me seems what is easier to deal with (and how)? Yes, asking users to do #undef I seems in principle the best thing. But I am not certain it is the path of least effort for downstream.
Can we e.g. document those cryptic errors well enough that #undef I is easy to find for affected projects (or do they look different everywhere)!?

@rgommers

SciPy apparently had some failures with boost using I in some places and there was an argument that others might run into that as well. @rgommers seemed to think others projects are likely to run into that particular thing.

Here is the relevant discussion: scipy/scipy#19726. tl;dr it shouldn't have been a problem in C++, but in practice it is due to various bugs. Boost.Math was fixed in Dec'23, MS UCRT is only fixed when compiling for C++17 and up: microsoft/STL#4281.

Overall, I do not think this is a big enough problem that we shouldn't just do what seems best.

On second thought, I think I agree. It's annoying to flip-flop on a fix, but it only affects people who build from source, which isn't nearly as bad as causing a problem at runtime. Either way, I think we should have the same state for 2.0.x and 2.1.x, because downstream packages are going to support both for quite a while, and having to deal with both situations depending on numpy version is worse than any consistent choice.

So what's the decision here? I think there is clear opposition to keeping the new state (I undeclared). It is unclear to me that there is any meaningful preference for keeping the #undef I, but I may be missing it.
The idea of adding an NPY_... option seems mainly related to opting out of the #undef I.

If nobody comments, I have to assume the decision is to remove the #undef I again and try to put the error message somewhere (maybe backport it into the migration guide complex section or just in release notes).

And this will need be backported to both 2.1 and 2.0.2.

try to put the error message somewhere (maybe backport it into the migration guide complex section or just in release notes).

#27105 documents the current situation. I don't have a strong opinion of whose code to break: those who use I freely (like boost) vs. those who expect I to be defined.

EDIT(seberg): Fix PR link.

If nobody comments, I have to assume the decision is to remove the #undef I again and try to put the error message somewhere (maybe backport it into the migration guide complex section or just in release notes).

It looks to me like on balance there is more support for this option, so I agree we should go that way.

It looks to me like on balance there is more support for this option

It is probably the best option long term, the undef is just a short term solution.

REV: Revert undef I and document it

Navigation Menu

Repository navigation

BUG: Compiling C extension against numpy 2.0.1 fails with `error: 'I' undeclared` #27083

Describe the issue:

Reproduce the code example:

Error message:

Python and NumPy Versions:

Runtime Environment:

Context for the issue:

3 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

BUG: Compiling C extension against numpy 2.0.1 fails with error: 'I' undeclared #27083

Description

Describe the issue:

Reproduce the code example:

Error message:

Python and NumPy Versions:

Runtime Environment:

Context for the issue:

Activity

WarrenWeckesser commented on Jul 30, 2024

mattip commented on Jul 30, 2024

duncanmmacleod commented on Jul 30, 2024

mattip commented on Jul 30, 2024

moorepants commented on Jul 30, 2024

mattip commented on Jul 30, 2024

mattip commented on Jul 30, 2024

moorepants commented on Jul 30, 2024

tiran commented on Aug 4, 2024

mattip commented on Aug 4, 2024

tiran commented on Aug 4, 2024

mattip commented on Aug 4, 2024

tiran commented on Aug 4, 2024

raffienficiaud commented on Aug 5, 2024

3 remaining items

raffienficiaud commented on Aug 6, 2024

seberg commented on Aug 6, 2024

rgommers commented on Aug 6, 2024

seberg commented on Aug 8, 2024

mattip commented on Aug 8, 2024

rgommers commented on Aug 9, 2024

charris commented on Aug 9, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

BUG: Compiling C extension against numpy 2.0.1 fails with `error: 'I' undeclared` #27083