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

API: deprecate compat and selected lib utils #23830

Merged
merged 3 commits into from Jun 14, 2023

Conversation

mtsokol
Copy link
Member

@mtsokol mtsokol commented May 28, 2023

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
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.

numpy/__init__.py Outdated Show resolved Hide resolved
numpy/__init__.py Outdated Show resolved Hide resolved
pytest.ini Outdated Show resolved Hide resolved
numpy/__init__.py Outdated Show resolved Hide resolved
@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)

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.

numpy/core/_methods.py Show resolved Hide resolved
pytest.ini Outdated Show resolved Hide resolved
@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
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

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?

@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 ...

@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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants