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

BUG: Prevent crash on repr of recursive array #8963

Merged
merged 2 commits into from
Apr 25, 2017

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Apr 20, 2017

Fixes #8960, by:

  • Forbidding reentrance of array2string (within a single thread)
  • Not calling np.maximum on arrays which might be recursive - there's no point setting up an IntegerFormatter if we're not yet sure we're working with integers

Previously, formatters could incur errors from being run on object arrays, even
though the formatter was not used.
@@ -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:
Copy link
Member

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)

Copy link
Member Author

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

Copy link
Member

@ahaldane ahaldane left a 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='...'):
Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member

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.



# gracefully handle recursive calls - this comes up when object arrays contain
# themselves
Copy link
Member Author

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
"""
Copy link
Member Author

Choose a reason for hiding this comment

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

Docstring improved too.

@ahaldane
Copy link
Member

LGTM. I'm going to go ahead and merge. Thanks @eric-wieser !

@ahaldane ahaldane merged commit 29e8883 into numpy:master Apr 25, 2017
@eric-wieser
Copy link
Member Author

Great! Let's see if this makes #8983 easier

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