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

MAINT: Move export for scipy arm64 helper into main module #22972

Merged
merged 2 commits into from Jan 9, 2023

Conversation

charris
Copy link
Member

@charris charris commented Jan 8, 2023

Backport of #22939.

This is a follow up to gh-22679 which addressed gh-22673.

The main thing is that we want the functions to be available after importing NumPy, so they need to be part of multiarray. However, npymath is a static library, so the symbols are not really exported there. The former PR did actually work in practice but this seems like it is technically the right place?

For some reason, I had to add nextafter to be able to do:

from scipy.spatial.distance import euclidean

with the SciPy 1.9.3 wheels. SciPy test collection works with this for the 1.9.3 wheel, so this should be all the symbols hopefully.

@charris charris added 03 - Maintenance 08 - Backport Used to tag backport PRs labels Jan 8, 2023
@charris charris added this to the 1.24.2 release milestone Jan 8, 2023
@charris
Copy link
Member Author

charris commented Jan 9, 2023

The failure seem due to

2023-01-08T22:50:53.2026876Z numpy/core/src/umath/loops.c.src:1497:31: error: implicit declaration of function ‘npy_nextafter’; did you mean ‘npy_nextafterl’? [-Werror=implicit-function-declaration]

This is a follow up to numpygh-22679 which addressed numpygh-22673.

The main thing is that we want the functions to be available after
importing NumPy, so they need to be part of multiarray.
However, `npymath` is a static library, so the symbols are not really
exported there.  The former PR did actually work in practice but this
seems like it is technically the right place?

For some reason, I had to add nextafter to be able to do:

    from scipy.spatial.distance import euclidean

with the SciPy 1.9.3 wheels.  SciPy test collection works with this for
the 1.9.3 wheel, so this should be all the symbols hopefully.
@charris
Copy link
Member Author

charris commented Jan 9, 2023

I suspect there is a missing backport.

#define npy_asinh asinh
#define npy_copysign copysign
#define npy_log1p log1p
#endif
double npy_nextafter(double x, double y);
Copy link
Member

Choose a reason for hiding this comment

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

I restored this line which should not be removed

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks. So that is why I had to add nextafter, but it was previously no issue. I guess it could be excluded from the other file in that case, but seems it doesn't matter?

Copy link
Member

Choose a reason for hiding this comment

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

It needs to be in both files for this branch. One is compiled into libnpy_math.a, this is the one that is removed from main but on this branch it is alive and well. The other implementation is exported from the main numpy namespace because of <mumble>macos arm64 build linking problem</mumble>

Copy link
Member Author

@charris charris Feb 4, 2023

Choose a reason for hiding this comment

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

@seberg @mattip

This seems to be causing problems with the macosx m1 wheels, the linker complains of two symbols _npy_nextafter, which I suspect is because of long double being double on the m1 platform. This doesn't seem to be a problem in main.

EDIT: This is when building wheels for 1.24.2.

Copy link
Member

Choose a reason for hiding this comment

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

Arrgg, I guess that because npy_nextafter is already here, it should not have been included in arm64_exports.c for the backport...

@charris charris merged commit 30535f4 into numpy:maintenance/1.24.x Jan 9, 2023
@charris charris deleted the backport-22939 branch January 9, 2023 13:58
@charris
Copy link
Member Author

charris commented Jan 9, 2023

Thanks Matti.

charris added a commit to charris/numpy that referenced this pull request Feb 4, 2023
This reverts commit 30535f4, reversing
changes made to 9b01523.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
03 - Maintenance 08 - Backport Used to tag backport PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants