Skip to content

MAINT: ndarray.__repr__ should not rely on __array_function__ #12212

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 3 commits into from
Oct 27, 2018

Conversation

shoyer
Copy link
Member

@shoyer shoyer commented Oct 18, 2018

ndarray.__repr__ and ndarray.__str__ should not rely upon __array_function__ internally, so they are still well defined on subclasses even if array_repr and array_str are not implemented.

Fixes gh-12162

``ndarray.__repr__`` and ``ndarray.__str__`` should not rely upon
``__array_function__`` internally, so they are still well defined on subclasses
even if ``array_repr`` and ``array_str`` are not implemented.

Fixes numpygh-12162
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.

The unit test is certainly sensible -- I tried this locally a few days ago.

Probably out of scope, but I wonder what happens if you set a custom array_repr() that simply points to the one used by ndarray, but then write your own array_add. Would the repr padding calculations (which sometimes use the array add) then go through the custom array_add instead of the ndarray version?

@shoyer
Copy link
Member Author

shoyer commented Oct 23, 2018

Probably out of scope, but I wonder what happens if you set a custom array_repr() that simply points to the one used by ndarray, but then write your own array_add. Would the repr padding calculations (which sometimes use the array add) then go through the custom array_add instead of the ndarray version?

We don't expose the non-overloaded version of array_repr() externally, so I don't think this is possible.

@ahaldane
Copy link
Member

ahaldane commented Oct 26, 2018

I like that this splits up the numpy array_repr api and the array_repr implementation. I mentioned in another thread that ducktype implementors may want to reuse the implementation (after some changes to array2string), but before now there was no way to call it separately from the api method.

I still have a feeling this isn't the last change we will make here. For instance, I'm not 100% convinced that array_repr, array_str and array2string should be part of the __array_function__ api. Should they be part of the _array_function__-api, or are they simply one-off helper functions for __repr__ code? Does the fact that they nearly already work as-is when called on ducktypes suggest the latter? When do we ever want a ducktype's __repr__ to be different from its array_repr? I also find the arrays2string vs array2string.__wrapper__ juggling here convoluted and a sign that something isn't quite correctly organized, though I understand the technical reason for it.

Despite all that, this PR seems like a step forward for now, to get things in a working state, so I would merge it.

(Needs rebase though).

@shoyer
Copy link
Member Author

shoyer commented Oct 26, 2018

Should they be part of the array_function_-api, or are they simply one-off helper functions for repr code? Does the fact that they nearly already work as-is when called on ducktypes suggest the latter? When do we ever want a ducktype's repr to be different from its array_repr?

"One-off helper functions" seems cleaner to me, but already expose them in NumPy's top level API which suggests they need __array_function__.

If I were starting from scratch I would probably only expose arary2string in NumPy's top level namespace, which would make this whole thing cleaner.

@ahaldane
Copy link
Member

All right, LGTM, merging. Thanks Stephan.

@ahaldane ahaldane merged commit 45718fd into numpy:master Oct 27, 2018
@charris charris changed the title MAINT: ndarray.__repr__ should not rely on __array_function__ MAINT: ndarray.__repr__ should not rely on __array_function__ Nov 10, 2018
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