E52A API: deprecate compat and selected lib utils by mtsokol · Pull Request #23830 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

API: deprecate compat and selected lib utils #23830

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.

Sign up for GitHub

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

Jump to bottom
Merged
rgommers merged 3 commits into from
Jun 14, 2023
Merged

API: deprecate compat and selected lib utils #23830

rgommers merged 3 commits into from
Jun 14, 2023

Conversation

@mtsokol
Copy link
Member
@mtsokol mtsokol commented May 28, 2023 •
edited
Loading

Hi @rgommers,

Here I share a WIP PR with a deprecation of a few positions mentioned in #23537 for the purpose of cleaning up main namespace API.

Here I deprecate compat module raising a warn on import, and deprecate some functions from lib (listed in #12385). Functions are deprecated in such way: When they are imported from the main namespace a warn is printed, when they are called an exception is raised, saying that these methods are expired and they can be used directly from numpy.lib (the financial functions are deprecated this way).
When these functions are imported directly from numpy.lib or numpy.lib.utils and called a warn is raised.

@mtsokol mtsokol force-pushed the deprecate-compat-and-lib-utils branch from 68a0d89 to 63b5edc Compare May 28, 2023 18:41
rgommers
rgommers reviewed May 29, 2023
View reviewed changes
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.

Thanks for picking this up @mtsokol! A few comments to tweak the approach.

@mtsokol mtsokol force-pushed the deprecate-compat-and-lib-utils branch 6 times, most recently from a82a435 to 376b840 Compare June 10, 2023 18:49
@mtsokol mtsokol force-pushed the deprecate-compat-and-lib-utils branch from 376b840 to 2f36ac5 Compare June 10, 2023 19:20
@mtsokol mtsokol marked this pull request as ready for review June 10, 2023 20:11
@mtsokol
Copy link
Member Author
mtsokol commented Jun 10, 2023

Hi @rgommers,
I pushed new changes with addressed review comments. In the current version the codebase doesn't use/depend on np.compat, so in the future release it can be removed by just dropping the module itself.

lib/utils methods covered in this PR are deprecated, but still available and used (therefore I silenced warnings caused by deprecations in pytest.ini)

rgommers
rgommers reviewed Jun 12, 2023
View reviewed changes
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.

This looks great, and is very close to mergeable - only two more comments.

@mtsokol mtsokol force-pushed the deprecate-compat-and-lib-utils branch 2 times, most recently from b119f44 to 83636a6 Compare June 14, 2023 07:08
@mtsokol mtsokol force-pushed the deprecate-compat-and-lib-utils branch from 83636a6 to 22d9ffb Compare June 14, 2023 13:48
@mtsokol mtsokol requested a review from rgommers June 14, 2023 15:39
rgommers
rgommers approved these changes Jun 14, 2023
View reviewed changes
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.

LGTM now and all green, in it goes. Thanks again @mtsokol!

@rgommers
Copy link
Member
rgommers commented Jun 14, 2023

I'll squash-merged this so it's easy to backport to 1.26.x once that branch gets created. @charris I think this is the first PR that should end up in 1.26.0. Do you have a preference for how to set milestone/labels here to keep track of things for that release?

@rgommers rgommers merged commit 12efa8e into numpy:main Jun 14, 2023
@mtsokol mtsokol deleted the deprecate-compat-and-lib-utils branch June 14, 2023 17:32
@rgommers rgommers mentioned this pull request Jun 19, 2023
Closed
18 tasks
@glemaitre glemaitre mentioned this pull request Jun 21, 2023
Merged
@pllim pllim mentioned this pull request Jun 21, 2023
Closed
@mtsokol mtsokol mentioned this pull request Jul 10, 2023
Merged
@lesteve
Copy link
Contributor
lesteve commented Jul 10, 2023

It turns out np.byte_bounds is used in joblib and has actually been documented since numpy 1.18 (done in #13847). This PR is causing failures in the scikit-learn CI build where we test against the numpy development version (and turn most warnings into errors). From the linked issues, astropy had seen similar failures.

My understanding is that NEP 52 aims at cleaning up the main namespace. I would have expected that np.byte_bounds would be deprecated but not necessarily np.lib.byte_bounds.

I see a two main ways forward (there may be others):

  • np.lib.byte_bounds will be removed eventually, so we need to reimplement byte_bounds in joblib if we actually need it. The code of np.lib.byte_bounds seems simple enough so it should not be too much work.
  • np.lib.byte_bounds is going to stay, so it needs to be "undeprecated" in numpy and we need to use it in joblib rather than the deprecated np.byte_bounds

As a side-comment, the list of undocumented functions in the main namespace from #12385 is from November 2018 and is not fully accurate, for example #13847 added documentation for other functions, like deprecate, deprecate_with_doc, etc ...

@lesteve lesteve mentioned this pull request Jul 10, 2023
Closed
@seberg
Copy link
Member
seberg commented Jul 10, 2023

@lesteve it looks to me like np.may_share_memory(arr1, arr2) is a better replacement anyway. np.lib. is mainly an implementation detail of NumPy, so keeping things there is a dedicated decision.

I agree that this deprecation probably should have mentioned the suggested alternative.

@rgommers
Copy link
Member
rgommers commented Jul 12, 2023

Adding the forward link: gh-24154 un-deprecates np.byte_bounds for now. It seems like we agree that it shouldn't be in the main namespace; it could be useful to find it a new home. We'll need to sort out how numpy.lib is structured before that can be done, there's no good namespace for it right now.

@mtsokol mtsokol mentioned this pull request Aug 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seberg seberg seberg left review comments

@rgommers rgommers rgommers approved these changes

Assignees

No one assigned

Labels

30 - API Numpy 2.0 API Changes

Projects

NumPy first-time contributor PRs
Status: Completed

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mtsokol @rgommers @lesteve @seberg @ngoldbaum
0