-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
API: Make descr->f only accessible through PyDataType_GetArrFuncs
#25897
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
Conversation
Ping @lithomas1 for pandas too |
* vendor in the latest version of the compat header from: numpy/numpy#25897 * with this adjustment and that branch, the following scenarios all have full test suite passing: - build + test against his feature branch with new header vendored into my branch - (with new header vendored in) build against his branch, test against 1.26.4 - (with new header vendored in) build + test against 1.26.4 [skip cirrus] [skip circle]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pasting from my commit message in scipy/scipy#20108:
* vendor in the latest version of the compat header from:
https://github.com/numpy/numpy/pull/25897
* with this adjustment and that branch, the following
scenarios all have full test suite passing:
- build + test against his feature branch with new header vendored
into my branch
- (with new header vendored in) build against his branch, test against 1.26.4
- (with new header vendored in) build + test against 1.26.4
I think that's pretty good?
I pinged Thomas and he said he'd have a chance to look at the impact of this on pandas this weekend. Let's try to give pandas a chance to add the header in a PR that can be quickly merged before we merge this so there's a minimum amount of time where the numpy tests are broken because pandas is broken. I can't find any other maintained projects on github code search that this change will break, so once Pandas is ready I think we can merge this. Please let me know if there are issues with that plan. |
pandas-dev/pandas#57668 should fix pandas. |
@lithomas1 let us know when you're ready and I'll merge this |
We're hopefully good on the pandas side. |
Nice, Pandas could stop using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just retested this with SciPy (with Tyler's changes), that works cleanly with this branch and also building against this branch and then running the SciPy test suite against NumPy 1.22.4 looks good.
Changes in the fixup commit LGTM, and the rest was already reviewed. So let's give this another go. Thanks @seberg & reviewers!
* vendor in the latest version of the compat header from: numpy/numpy#25897 * with this adjustment and that branch, the following scenarios all have full test suite passing: - build + test against his feature branch with new header vendored into my branch - (with new header vendored in) build against his branch, test against 1.26.4 - (with new header vendored in) build + test against 1.26.4 [skip cirrus] [skip circle]
Fixes gh-20107 Vendor in the latest version of the compat header from: numpy/numpy#25897 The following scenarios all have full test suite passing: - build + test against his feature branch with new header vendored into my branch - (with new header vendored in) build against his branch, test against 1.26.4 - (with new header vendored in) build + test against 1.26.4 [skip cirrus] [skip circle]
This is the reversion of the reversion. With a follow up commit to move the definitions which now require
GetArrFuncs
. Together with the other PR to make surenpy_2_compat.h
is vendorable, this should make @tylerjereddy PR on SciPy work fine (I wanted to try it but ran into issues, so just opening for now no tested with SciPy).