Skip to content

Conversation

WarrenWeckesser
Copy link
Member

@WarrenWeckesser WarrenWeckesser commented Jul 9, 2024

When building scipy with numpy 1.26.4, the following compiler warning is generated:

In file included from ../scipy/signal/_splinemodule.cc:5: ../scipy/signal/_splinemodule.h:13: warning: "PyArray_MIN" redefined
   13 | #define PyArray_MIN(a, b) (((a) < (b)) ? (a) : (b))
      |
In file included from ../../../../../py3.12.4/lib/python3.12/site-packages/numpy/core/include/numpy/ndarrayobject.h:12,
                 from ../../../../../py3.12.4/lib/python3.12/site-packages/numpy/core/include/numpy/arrayobject.h:5,
                 from ../scipy/signal/_splinemodule.cc:2:
../../../../../py3.12.4/lib/python3.12/site-packages/numpy/core/include/numpy/ndarraytypes.h:964: note: this is the location of the previous definition
  964 | #define PyArray_MIN(a,b) (((a)<(b))?(a):(b))

In numpy 2.0.0, PyArray_MIN is defined in numpy/npy_math.h, so it doesn't result in the redefinition problem. In numpy 1.26.4, PyArray_MIN is defined in numpy/ndarraytypes.h, which is transitively included by the include of numpy/arrayobject.h in _splinemodule.cc, resulting in the redefinition warning.

To fix this, the PyArray_MIN macro is replaced with std::min.

@WarrenWeckesser WarrenWeckesser added scipy.signal maintenance Items related to regular maintenance tasks C/C++ Items related to the internal C/C++ code base labels Jul 9, 2024
@ev-br
Copy link
Member

ev-br commented Jul 9, 2024

LGTM assuming the CI agrees.

(IIRC the macro was needed in the header file, in one of the iterations of #20519 at least; great if it's no longer needed)

@lucascolley lucascolley changed the title MAINT: signal: Don't redefine PyArray_MIN macro. MAINT: signal: Don't redefine PyArray_MIN macro Jul 9, 2024
@lucascolley lucascolley added this to the 1.15.0 milestone Jul 9, 2024
@ilayn
Copy link
Member

ilayn commented Jul 9, 2024

Should we protect the definition with ifndef just in case? Can't think of a valid use case but you never know.

@WarrenWeckesser
Copy link
Member Author

After further ponderation: this is C++, so instead of rolling our own min function/macro, let's use std::min. I'll push an update.

When building scipy with numpy 1.26.4, the following compiler
warning is generated:

In file included from ../scipy/signal/_splinemodule.cc:5:
../scipy/signal/_splinemodule.h:13: warning: "PyArray_MIN" redefined
   13 | #define PyArray_MIN(a, b) (((a) < (b)) ? (a) : (b))
      |
In file included from ../../../../../py3.12.4/lib/python3.12/site-packages/numpy/core/include/numpy/ndarrayobject.h:12,
                 from ../../../../../py3.12.4/lib/python3.12/site-packages/numpy/core/include/numpy/arrayobject.h:5,
                 from ../scipy/signal/_splinemodule.cc:2:
../../../../../py3.12.4/lib/python3.12/site-packages/numpy/core/include/numpy/ndarraytypes.h:964: note: this is the location of the previous definition
  964 | #define PyArray_MIN(a,b) (((a)<(b))?(a):(b))

In numpy 2.0.0, PyArray_MIN is defined in numpy/npy_math.h, so
it doesn't result in the redefinition problem. In numpy 1.26.4,
PyArray_MIN is defined in numpy/ndarraytypes.h, which is
transitively included by the include of numpy/arrayobject.h
in _splinemodule.cc, resulting in the redefinition warning.

To fix this, the PyArray_MIN macro is replaced with std::min.
@WarrenWeckesser WarrenWeckesser force-pushed the signal-fix-build-warning branch from b17e74f to 58aee83 Compare July 9, 2024 16:32
Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Warren!

@ev-br ev-br merged commit 2db2177 into scipy:main Jul 9, 2024
@WarrenWeckesser WarrenWeckesser deleted the signal-fix-build-warning branch July 9, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C/C++ Items related to the internal C/C++ code base maintenance Items related to regular maintenance tasks scipy.signal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants