-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: Prevent crash on repr of recursive array #8963
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
Conversation
Previously, formatters could incur errors from being run on object arrays, even though the formatter was not used.
be255d6
to
c3b84e7
Compare
@@ -455,7 +495,7 @@ def array2string(a, max_line_width=None, precision=None, | |||
lst = format_function(arr[0]) | |||
else: | |||
lst = style(x) | |||
elif reduce(product, a.shape) == 0: | |||
elif functools.reduce(product, a.shape) == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of this? (forgive my ignorance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not polluting the global namespace. Previously there was also from functools import reduce
.
As for why that's there - reduce
is no longer a global in python 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lambda part looks good.
The wrapper part seems fine too. It doesn't protect against recursion where 'self' changes each iteration, as in:
>>> np.set_printoptions(formatter={'all': lambda x: str(np.array([x]))})
but I don't think we need that.
@@ -337,6 +354,29 @@ def _array2string(a, max_line_width, precision, suppress_small, separator=' ', | |||
return lst | |||
|
|||
|
|||
def _recursive_guard(fillvalue='...'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to add a comment here that this is to account for (for example) self-containing object arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - I'll fix that up when I find the time (or else, you can if you don't want to wait - this PR is editable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, I will wait. Otherwise it looks good to merge to me.
c3b84e7
to
1e65a2a
Compare
|
||
|
||
# gracefully handle recursive calls - this comes up when object arrays contain | ||
# themselves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahaldane: Done - seemed more appropriate to comment "why" at the call-site
argument, it returns `fillvalue` instead of recursing. | ||
|
||
Largely copied from reprlib.recursive_repr | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring improved too.
LGTM. I'm going to go ahead and merge. Thanks @eric-wieser ! |
Great! Let's see if this makes #8983 easier |
Also adds a release note for numpygh-8963
Also adds a release note for numpygh-8963
Fixes #8960, by:
array2string
(within a single thread)np.maximum
on arrays which might be recursive - there's no point setting up anIntegerFormatter
if we're not yet sure we're working with integers