Skip to content

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

Merged
merged 2 commits into from
Mar 3, 2024

Conversation

seberg
Copy link
Member

@seberg seberg commented Feb 27, 2024

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 sure npy_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).

@ngoldbaum
Copy link
Member

Ping @lithomas1 for pandas too

tylerjereddy added a commit to tylerjereddy/scipy that referenced this pull request Feb 27, 2024
* 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]
Copy link
Contributor

@tylerjereddy tylerjereddy left a 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?

@ngoldbaum
Copy link
Member

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.

@rgommers rgommers added this to the 2.0.0 release milestone Feb 28, 2024
@lithomas1
Copy link
Collaborator

pandas-dev/pandas#57668 should fix pandas.

@ngoldbaum
Copy link
Member

@lithomas1 let us know when you're ready and I'll merge this

@lithomas1
Copy link
Collaborator

We're hopefully good on the pandas side.

@rgommers
Copy link
Member

rgommers commented Mar 3, 2024

Nice, Pandas could stop using ->f complete, so that's problem solved. For SciPy we can't, so we're going to have to sync merges and wheel builds a bit. Looks like this is good to go though, and we can work through this later today.

Copy link
Member

@rgommers rgommers left a 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!

@rgommers rgommers merged commit 54b174f into numpy:main Mar 3, 2024
@seberg seberg deleted the revert-arrfuncs branch March 3, 2024 18:07
rgommers pushed a commit to tylerjereddy/scipy that referenced this pull request Mar 4, 2024
* 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]
rgommers pushed a commit to scipy/scipy that referenced this pull request Mar 4, 2024
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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants