Skip to content

API: Update lib.stride_tricks namespace #24580

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
Aug 30, 2023

Conversation

mtsokol
Copy link
Member

@mtsokol mtsokol commented Aug 29, 2023

Relevant issue #24507

Hi @rgommers @ngoldbaum,

This PR moves lib.stride_tricks module to a private file and ensures that it's public methods are only available through the main or local namespaces. For local namespace there are as_strided and sliding_window_view functions.

@mtsokol mtsokol changed the title API: Update lib.stride_tricks namespace API: Update lib.stride_tricks namespace Aug 29, 2023
@mtsokol mtsokol self-assigned this Aug 29, 2023
@mtsokol mtsokol force-pushed the lib-stride-tricks-namespace branch from e275ece to 8de5f9a Compare August 29, 2023 11:42
@ngoldbaum
Copy link
Member

I see DummyArray is included in the public stride_tricks namespace as well. Searching, it does seem to have some downstream usage. However, its implementation is literally:

class DummyArray:
    """Dummy object that just exists to hang __array_interface__ dictionaries                                                                                         
    and possibly keep alive a reference to a base array.                                                                                                              
    """

    def __init__(self, interface, base=None):
        self.__array_interface__ = interface
        self.base = base

So IMO, we should make it private and add a note to the migration guide that downstream projects who need it should just copy/paste it into their source tree. There's no need to expand the docstrings or make public something so trivial to implement.

@mtsokol mtsokol force-pushed the lib-stride-tricks-namespace branch from 8de5f9a to 6e12461 Compare August 30, 2023 09:10
@mtsokol
Copy link
Member Author

mtsokol commented Aug 30, 2023

So IMO, we should make it private and add a note to the migration guide that downstream projects who need it should just copy/paste it into their source tree. There's no need to expand the docstrings or make public something so trivial to implement.

That makes sense to me. In the github search engine it shows only 13 hits, so I removed it from the public np.lib.stride_tricks namespace.

Copy link
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, are there any downstream usages that need to get resolved before this can be merged?

@ngoldbaum

This comment was marked as outdated.

@rgommers
Copy link
Member

Ah yeah, it does look like there are quite a few downstream usages.

That link doesn't show me anything. The numpy.lib.stride_tricks namespace is still present after this PR gets merged. Can you elaborate?

@ngoldbaum
Copy link
Member

That link doesn't show me anything. The numpy.lib.stride_tricks namespace is still present after this PR gets merged. Can you elaborate?

Oops, sorry, you're right, they're all usages of as_strided and sliding_window_view, never mind.

@ngoldbaum
Copy link
Member

OK, in that case, in it goes.

@ngoldbaum ngoldbaum merged commit b73a5ae into numpy:main Aug 30, 2023
@mtsokol mtsokol deleted the lib-stride-tricks-namespace branch August 30, 2023 16:37
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