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: PyArray_MAX no longer available in numpy/arrayobject.h #18966

Closed
tupui opened this issue Jul 26, 2023 · 10 comments · Fixed by #18967
Closed

BUG: PyArray_MAX no longer available in numpy/arrayobject.h #18966

tupui opened this issue Jul 26, 2023 · 10 comments · Fixed by #18967
Labels
C/C++ Items related to the internal C/C++ code base upstream bug Items related to bugs in upstream projects
Milestone

Comments

@tupui
Copy link
Member

tupui commented Jul 26, 2023

There was a change in NumPy which removes PyArray_MAX and PyArray_MIN. E.g. of issue in our CI https://github.com/scipy/scipy/actions/runs/5663008932/job/15343987040#step:9:2979

../scipy/integrate/_odepackmodule.c:477:12: error: implicit declaration of functionPyArray_MAX’; did you meanPyArray_Max’? [-Werror=implicit-function-declaration]
  477 |     *lrw = PyArray_MAX(lrn,lrs);
      |            ^~~~~~~~~~~
      |            PyArray_Max

I believe the PR on NumPy is numpy/numpy#24258

@tupui tupui added upstream bug Items related to bugs in upstream projects C/C++ Items related to the internal C/C++ code base labels Jul 26, 2023
@ev-br
Copy link
Member

ev-br commented Jul 26, 2023

Can you comment on why this change and not importing PyArray_MAX from npy_math.h, where the linked numpy PR moves them to?

@tupui
Copy link
Member Author

tupui commented Jul 26, 2023

I have no clue what's best to do 😅 I just followed the bug, C and NumPy's internal are not my expertise. I am happy to try to do the change you are suggesting instead. And seems like CI does not like what I did hum.

So I would need to do #include "numpy/npy_math.h" in both files??

@tupui
Copy link
Member Author

tupui commented Jul 26, 2023

But if we use npy_math.h then it's not backward compatible no?

@ev-br
Copy link
Member

ev-br commented Jul 26, 2023

These includes might need to be guarded by an #ifdef depending on the numpy version? Let's ask @lysnikolaou for a recommended way of working around the numpy change.

@ev-br
Copy link
Member

ev-br commented Jul 26, 2023

Or maybe it's easiest to just copy-paste the macros in their full complexity and not depend on numpy for them:

#define PyArray_MAX(a,b) (((a)>(b))?(a):(b))
#define PyArray_MIN(a,b) (((a)<(b))?(a):(b))

@tupui
Copy link
Member Author

tupui commented Jul 26, 2023

Let's ask yes 👍

For now locally I got it working with:

out = (PyArrayObject*)PyArray_Max(a, b, NULL);
out = (PyArrayObject*)PyArray_Min(a, b, NULL);

I updated the PR.

@lysnikolaou
Copy link
Contributor

I'm not an expert really, and with my limited understanding, I don't get why including "numpy/npy_math.h" is backwards-incompatible. If it is, then you can hide it behind an #ifdef, since the move will only be realeased in numpy 2.0.

Agreed that it might be best to copy-paste the macros though since they're not overly complex.

@tupui
Copy link
Member Author

tupui commented Jul 26, 2023

I don't get why including "numpy/npy_math.h" is backwards-incompatible.

We need to support NumPy >=1.22.4. So I would suppose that new changes in npy_math.h would not be backported/available on older versions. But I don't know how all that works.

I am happy to move the macros as there are just 2 files where these are used in our code base.

@lysnikolaou
Copy link
Contributor

So I would suppose that new changes in npy_math.h would not be backported/available on older versions.

They won't be backported but my feeling is that if you keep both the old #include and the new #include "numpy/npy_math.h" in, it should be okay on all of the supported versions.

Still better to just move the macros.

@tupui
Copy link
Member Author

tupui commented Jul 26, 2023

Ah yes i see what you both meant now! See my C level 😅

I will update my PR to just hard code the macro there then, thanks again!

@tupui tupui changed the title BUG: PyArray_MAX no longer available in NumPy BUG: PyArray_MAX no longer available in numpy/arrayobject.h Jul 26, 2023
@tupui tupui added this to the 1.12.0 milestone Jul 27, 2023
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 upstream bug Items related to bugs in upstream projects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants