8000 numpy array printing regression for ndarray subclasses in NumPy 1.14 · Issue #10360 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

numpy array printing regression for ndarray subclasses in NumPy 1.14 #10360

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.

Sign up for GitHub

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

Jump to bottom
Closed
ngoldbaum opened this issue Jan 10, 2018 · 13 comments
Closed

numpy array printing regression for ndarray subclasses in NumPy 1.14 #10360

ngoldbaum opened this issue Jan 10, 2018 · 13 comments
Labels
00 - Bug

Comments

@ngoldbaum
Copy link
Member
ngoldbaum commented Jan 10, 2018 •
edited by eric-wieser
Loading

It looks like some of the array printing changes in NumPy 1.14 are causing issues in yt. Specifically, printing yt's object that represents a scalar with units is triggering a recursion error. Here is a simplified example that is sufficient to trigger the same error:

import numpy as np


class MyArray(np.ndarray):
    def __new__(cls, inp):
        obj = np.asarray(inp).view(cls)
        return obj

    def __str__(self):
        return super(MyArray, self).__str__()

    def __getitem__(self, item):
        ret = super(MyArray, self).__getitem__(item)
        return MyArray(ret)


if __name__ == "__main__":
    a = MyArray(1)
    print(a)

This script triggers an infinite recursion error on NumPy 1.14.0 and prints 1 on NumPy 1.13.3.

Is there any chance that this can be fixed on NumPy's side for a 1.14.1 release? I'm honestly not sure how to fix this on the yt side without making major changes to our ndarray subclasses.

Ping @mhvk
@ngoldbaum
Copy link
Member Author

Also apologies for not testing NumPy's beta and bringing this up before you shipped 1.14.0 :(

@eric-wieser eric-wieser added this to the 1.14.1 release milestone Jan 10, 2018
@eric-wieser
Copy link
Member

Thanks for the minimal test case. I'll try to take a look at this, since the blame is likely on me and/or @ahaldane

@eric-wieser
Copy link
Member
eric-wieser commented Jan 10, 2018 •
edited
Loading

This only happens on 0d arrays - because str(0d_array) falls back on str(0d_array[()]).

The idea here it to make the string of a 0d array equivalent to the string of the scalar.

In your case, it's not clear how to get hold of the scalar any more to do that. Perhaps we simply need recursion detection that calls self.view(ndarray) and retries.

@ngoldbaum
Copy link
Member Author

Yeah, this is sort of a quirk in our design, using an array subclass to represent a scalar. We probably shouldn't have done that but that ship sailed a long time ago. If there's a way to poison the recursion somehow that sounds like the best approach.

@ngoldbaum ngoldbaum mentioned this issue Jan 10, 2018
Closed
@eric-wieser
Copy link
Member

Here's what I think should happen:

  • We should catch RecursionError in ndarray.__str__, and downcast to retry, with a warning suggesting that maybe [()] is not returning a scalar, and therefore __str__ must be special-cased for 0d
  • You should add a special case for 0d __str__

@ngoldbaum
Copy link
Member Author
ngoldbaum commented Jan 10, 2018 •
edited
Loading

You should add a special case for 0d __str__

See yt-project/yt#1661

Thanks for the explanation.

@charris
Copy link
Member
charris commented Jan 10, 2018

Also apologies for not testing NumPy's beta and bringing this up before you shipped 1.14.0 :(

It's expected :) I think of the *.0 releases as the true betas, it's why I move to them so quickly these days. There just aren't enough folks who test the the rc's and waiting around is just a waste of time.

@mhvk
Copy link
Contributor
mhvk commented Jan 10, 2018

Note that this is a problem even if in the above __str__ is not overridden. Given that, if you subclass ndarray to add some new property to it, and you wish to support scalars, something like @ngoldbaum's implementation of recasting scalars as array scalars is the only possibility (we certainly use it in Quantity too), I think there will be a lot of other implementations that are going to have this problem, and it would seem wrong to even start giving warnings just for something as simple as doing print(mysubclass). Instead, I would suggest we do one of:

  1. Cast of array scalar to scalar via [()] only for ndarray itself, not for subclasses.
  2. Explicitly check that after [()], one now has an instance of a different class.

Option (2) is perhaps the safest, and also closest to the legacy behaviour.

p.s. It is just luck that in Quantity, we just create a string from the ndarray view and then add the unit string afterwards...

@eric-wieser
Copy link
Member
eric-wieser commented Jan 10, 2018 •
edited
Loading

Explicitly check that after [()], one now has an instance of a different class.

This unfortunately is not safe, as we could be indexing an object array. We already have questionable legacy behavior in ma.array due to making this assumption, I'd prefer to not introduce it elsewhere.

@mhvk
Copy link
Contributor
mhvk commented Jan 10, 2018 •
edited by eric-wieser
Loading

You mean an object array of arrays? I'm confused. Anyway, my main point really is that we should do something that does not cause a warning or exception. Something like the following should cover most cases:

# the str of 0d arrays is a special case: It should appear like a scalar,
# so floats are not truncated by `precision`, and strings are not wrapped
# in quotes. So we return the str of the scalar value.
if a.shape == ():
    item = a[()]
    # Many subclasses wrap any scalar output back into the
    # subclass. Avoid recursion for those (but allow arrays containing
    # arrays)
    if type(a) is np.ndarray or not isinstance(item, type(a)):
        return str(item)

@mhvk
Copy link
Contributor
mhvk commented Jan 10, 2018

Option 1 would be:

if type(a) is np.ndarray and a.shape == ():

Of course, an option (3) would be to add a recursion guard but just not raise a warning.

@charris
Copy link
Member
charris commented Feb 8, 2018

@eric-wieser @ahaldane What is the status of this for 1.14?

@ahaldane
Copy link
Member
ahaldane commented Feb 8, 2018

Just took a look at this. Let's try what @mhvk said, it seems reasonable.

@ahaldane ahaldane mentioned this issue Feb 8, 2018
Merged
ahaldane added a commit to ahaldane/numpy that referenced this issue Feb 11, 2018
@ahaldane
BUG: infinite recursion in str of 0d subclasses
ff12de3 
Fixes numpy#10360
hanjohn pushed a commit to hanjohn/numpy that referenced this issue Feb 15, 2018
@ahaldane @hanjohn
BUG: infinite recursion in str of 0d subclasses
f8ccee9 
Fixes numpy#10360
@charris charris mentioned this issue Feb 16, 2018
Merged
charris pushed a commit to charris/numpy that referenced this issue Feb 16, 2018
@ahaldane @charris
BUG: infinite recursion in str of 0d subclasses
6a05fea 
Fixes numpy#10360
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
00 - Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@charris @eric-wieser @mhvk @ngoldbaum @ahaldane
0