Skip to content

API: Update lib.polynomial and lib.npyio namespaces #24578

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 4 commits into from
Sep 1, 2023

Conversation

mtsokol
Copy link
Member

@mtsokol mtsokol commented Aug 29, 2023

Relevant issue #24507

Hi @rgommers @ngoldbaum,

This PR moves lib.polynomial and lib.npyio modules to private files and ensures that their public methods are only available through the main namespace. For lib.npyio there are two objects available from local namespace (they were used this way in docs and codebase) so for npyio there are _npyio_impl.py and npyio.py (and this is also the reason why so many lines changed - contents of npyio.py were moved to _npyio_impl.py).

@ngoldbaum
Copy link
Member

For future reference, it's a little easier to review renames if you make the rename a single commit and then subsequent edits to the file their own commit.

It looks like the two symbols this keeps in lib.npyio are BagObj and NpzFile. Both probably need to be public somewhere and np.lib.npyio is as good a place as any and has the benefit being where they publicly live now.

Given that this PR proposes keeping np.lib.npyio, It might make sense to deprecate (or just out-and-out move, since it's not used much) DataSource's location in the main namespace and move it to np.lib.npyio along with these two classes.

Can you add BagObj to the listing in doc/source/reference/routines.io.rst, along with NpzFile?

This removes 22 symbols from np.lib.__all__, all of them are in the main namespace:

In [5]: print(set(oldall) - set(np.lib.__all__))
{'save', 'polyint', 'poly1d', 'genfromtxt', 'polyadd', 'DataSource', 'savez_compressed', 'polyfit', 'savez', 'polymul', 'polysub', 'packbits', 'poly', 'roots', 'fromregex', 'loadtxt', 'load', 'polyval', 'unpackbits', 'polydiv', 'polyder', 'savetxt'}

In [6]: diff = set(oldall) - set(np.lib.__all__)

In [7]: diff.issubset(np.__all__)
Out[7]: True

@mtsokol mtsokol force-pushed the lib-polynomial-npyio-namespaces branch from 998e4c7 to 7be9485 Compare August 30, 2023 08:52
@mtsokol
Copy link
Member Author

mtsokol commented Aug 30, 2023

Given that this PR proposes keeping np.lib.npyio, It might make sense to deprecate (or just out-and-out move, since it's not used much) DataSource's location in the main namespace and move it to np.lib.npyio along with these two classes.

Sure! DataSource is originally available in np.lib.npyio so I only removed it from the main namespace and added an expiration message - where to find it now.

Can you add BagObj to the listing in doc/source/reference/routines.io.rst, along with NpzFile?

NpzFile is already there, in the "NumPy binary files" section. I added BagObj to a new "Object wrappers" section.

@mtsokol mtsokol requested a review from ngoldbaum August 30, 2023 08:56
@mtsokol mtsokol force-pushed the lib-polynomial-npyio-namespaces branch from 7be9485 to 1a082eb Compare August 30, 2023 17:34
@rgommers rgommers added this to the 2.0.0 release milestone Aug 31, 2023
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 good to me overall. My one question is why we'd want to make BagObj public? It doesn't seem like a thing that anyone should be using, nor can I find any interesting usage in the wild with code search right now.

@ngoldbaum
Copy link
Member

nor can I find any interesting usage in the wild with code search right now

Hmm, for some reason I thought I saw some when I searched the other day but can't find any now. Sorry for the mistaken suggestion!

The only public usage I can find outside of numpy is here and it's not even used in that file.

Given this level of usage I agree with Ralf there's no reason to add it to the docs, it's probably safe to just delete it since it has no internal consumers. Maybe add a custom AttributeError message if someone tries to directly access it that encourages people to copy/paste the old implementation if they do need it, it's less than 20 lines of non-comment code.

@mtsokol mtsokol force-pushed the lib-polynomial-npyio-namespaces branch from 1a082eb to 88885c5 Compare August 31, 2023 19:42
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.

All looks good to me now, let's get this in. The one CI failure is a network error - the Anaconda scientific-python bucket is struggling today.

@rgommers rgommers merged commit 63d9da8 into numpy:main Sep 1, 2023
@mtsokol mtsokol deleted the lib-polynomial-npyio-namespaces branch September 1, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants