Skip to content

DOC: Document array_function at a higher level. #13979

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 6 commits into from
Jul 14, 2019

Conversation

danielballan
Copy link
Contributor

Closes #13844

This PR adds a new document illustrating a very simple custom array container implementing __array_ufunc__ and __array_function__. It also rephrases the documentation on subclasses to direct users toward considering dispatch instead of inheritance.

Additional work is needed to restructure the documentation to make this new document easier for developers to discover and to provide more context about when to choose inheritance (rarely but sometimes still the right choice) or dispatch. I propose to defer that larger effort to a separate PR and get this published soon so that some high-level documentation on __array_function__ is at least available.

Feel free to take a heavy hand to this in reviewing/editing. I wrote this with the perspective of someone who has been following the __array_function__ conversation but has not thought deeply about the design.

@shoyer Any input of yours on this would be especially welcome.

@charris charris changed the title Array function high level docs DOC: Document array_function at a higher level. Jul 14, 2019
@charris
Copy link
Member

charris commented Jul 14, 2019

CircleCI is complaining.

/home/circleci/repo/doc/source/reference/arrays.classes.rst:9: WARNING: Mismatch: both interpreted text role prefix and reference suffix.

Subclassing a ``numpy.ndarray`` is possible but if your goal is to create an
array with *modified* behavior, as do dask arrays for distributed computation
and cupy arrays for GPU-based computation, subclassing is discouraged. Instead,
using numpy's :ref:`dispatch mechanism <basics.dispatch>`_ is recommended.
Copy link
Member

Choose a reason for hiding this comment

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

The trailing _ (signifying an external link) is not needed, it clashes with the leading :ref: which requests sphinx to create an link to internal documentation

provides all argument types with an ``'__array_function__'`` attribute.
This allows implementors to quickly identify cases where they should defer
to ``__array_function__`` implementations on other arguments.
Implementaitons should not rely on the iteration order of ``types``.
Copy link
Member

Choose a reason for hiding this comment

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

typo Implementations

- If all ``__array_function__`` methods return ``NotImplemented``,
NumPy will raise ``TypeError``.

If no ``__array_function__`` methods exist, NumPy will default to calling
Copy link
Member

Choose a reason for hiding this comment

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

exists

Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

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

Wow, thank you @danielballan, this is a really fantastic start!

I have a few minor suggestions, but I agree that we should try to merge this ASAP so that we have some documentation rather than none!

end. One way to simplify the question is by asking yourself if the
object you are interested in can be replaced as a single array or does
it really require two or more arrays at its core.
Subclassing a ``numpy.ndarray`` is possible but if your goal is to create an
Copy link
Member

Choose a reason for hiding this comment

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

I agree that the introduction to this page doesn't quite make sense, but I'm not sure this version entirely works either -- the next paragraph doesn't quite make sense without an introduction first. Maybe better to just add brief .. note:: with a link for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Reworking this properly is a bigger job. I have restored the original introduction, with only minor edits, and placed a note at the top directing users to consider dispatch instead of subclassing.

@danielballan
Copy link
Contributor Author

Thanks for the fast review, all!

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Jul 14, 2019
@charris charris added this to the 1.17.0 release milestone Jul 14, 2019
@charris charris merged commit 5edd794 into numpy:master Jul 14, 2019
@charris
Copy link
Member

charris commented Jul 14, 2019

Thanks @danielballan .

@charris charris modified the milestones: 1.17.0 release, 1.16.5 release Jul 14, 2019
@rgommers
Copy link
Member

this is great, thanks again Dan!

@danielballan danielballan deleted the array-function-high-level-docs branch July 15, 2019 02:54
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jul 18, 2019
@charris charris removed this from the 1.16.5 release milestone Jul 18, 2019
- ``inputs``, which could be a mixture of different types
- ``kwargs``, keyword arguments passed to the function

For this example we will only handle the method ``'__call__``.
Copy link
Contributor

Choose a reason for hiding this comment

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

extra single quote here

calls ``numpy.sum(self)``, and the same for ``mean``.

>>> @implements(np.sum)
... def sum(a):
Copy link
Contributor

Choose a reason for hiding this comment

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

a -> arr

... return arr._i * arr._N
...
>>> @implements(np.mean)
... def sum(a):
Copy link
Contributor

Choose a reason for hiding this comment

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

a -> arr

@thewtex
Copy link
Contributor

thewtex commented Aug 14, 2019

@danielballan thank you so much for writing this up! 🥇

@thewtex
Copy link
Contributor

thewtex commented Aug 14, 2019

#14272 addresses the minor issues identified

@danielballan
Copy link
Contributor Author

Thanks for the close read and cleanup, @thewtex! Glad it was helpful.

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.

high-level documentation for __array_function__ missing
6 participants