Skip to content

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.
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

@eric-wieser eric-wieser Apr 24, 2017

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.

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.

2 participants