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

DOC: Fix command in "Writing custom array containers" guide #18235

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

NicolasHug
Copy link
Contributor

This is a minor doc fix: the arr object is of type DiagonalArray, not ndarray.
I believe the point of this section is to illustrate that type(np.multiply(arr, 2)) == np.ndarray

@rossbar
Copy link
Contributor

rossbar commented Jan 26, 2021

The documentation is correct in its current form. The following paragraphs explain the difference between __array__ and other methods like __array_ufunc__ and __array_function__ in terms of how output types are handled.

@NicolasHug
Copy link
Contributor Author

Allow me to insist:

  • the arr object is of type DiagonalArray, not ndarray, so the current block is incorrect.
  • The following paragraph explains how one can make np.multiply return a DiagonalArray instead of a ndarray:

How can we pass our custom array type through this function?...

I believe that the current version assumes that arr is the output from np.multiply above, but it's not.

@rossbar
Copy link
Contributor

rossbar commented Jan 26, 2021

I believe that the current version assumes that arr is the output from np.multiply above, but it's not.

The output of np.multiply(arr, 2) is the object returned by __array__, i.e. the result of self._i * np.eye(self._N): np.eye creates an ndarray object.

Broken into finer steps:

>>> arr = DiagonalArray(5, 1)
>>> type(arr)
__main__.DiagonalArray
>>> b = np.multiply(arr, 2)
>>> type(b)
numpy.ndarray

@NicolasHug
Copy link
Contributor Author

@rossbar I understand all that but please read the document. It goes like this:

>>> arr = DiagonalArray(5, 1)
>>> type(arr)
__main__.DiagonalArray
>>> np.multiply(arr, 2)
>>> type(arr)
numpy.ndarray

which is wrong. Note how the output of np.multiply is not arr and it's not assigned to anything.

@rossbar
Copy link
Contributor

rossbar commented Jan 26, 2021

Just to be clear, type(np.multiply(arr, 2)) is calling type() on the output of np.multiply(arr, 2), which, as you point out, is not arr. Thus the original docs are correct.

In your above example, you have

>>> np.multiply(arr, 2)
>>> type(arr)

Which would be incorrect, but is different than the original documentation.

@NicolasHug
Copy link
Contributor Author

Thus the original docs are correct.

Sorry, no

but is different than the original documentation.

No, it's not different. Are we looking at the same document?

@NicolasHug
Copy link
Contributor Author

Here are the commands that are present in the master branch of the docs:

In [1]: import numpy as np

In [2]: class DiagonalArray:
   ...: ...     def __init__(self, N, value):
   ...: ...         self._N = N
   ...: ...         self._i = value
   ...: ...     def __repr__(self):
   ...: ...         return f"{self.__class__.__name__}(N={self._N}, value={self._i})"
   ...: ...     def __array__(self):
   ...: ...         return self._i * np.eye(self._N)
   ...: 

In [3]: arr = DiagonalArray(5, 1)

In [4]: arr
Out[4]: DiagonalArray(N=5, value=1)

In [5]: np.asarray(arr)
Out[5]: 
array([[1., 0., 0., 0., 0.],
       [0., 1., 0., 0., 0.],
       [0., 0., 1., 0., 0.],
       [0., 0., 0., 1., 0.],
       [0., 0., 0., 0., 1.]])

In [6]: np.multiply(arr, 2)
Out[6]: 
array([[2., 0., 0., 0., 0.],
       [0., 2., 0., 0., 0.],
       [0., 0., 2., 0., 0.],
       [0., 0., 0., 2., 0.],
       [0., 0., 0., 0., 2.]])

In [7]: type(arr)
Out[7]: __main__.DiagonalArray. # <------ This is not ndarray unlike the docs claim!!!!!!!!!

@rossbar
Copy link
Contributor

rossbar commented Jan 26, 2021

Wow - huge brainfart on my part - I just failed to interpret the diff correctly (I was assuming your change was what was already in the docs). So when I say "the original docs are correct", what I meant was "your original proposed changes are correct!

Sorry for all the noise.

@numpy numpy deleted a comment from NicolasHug Jan 26, 2021
@rossbar
Copy link
Contributor

rossbar commented Jan 26, 2021

No wonder I couldn't figure out why we were disagreeing even though I felt like we were saying the same thing :) - thanks for bearing with me @NicolasHug

@rossbar rossbar merged commit 8ef114b into numpy:master Jan 26, 2021
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.

None yet

2 participants