Skip to content

MAINT: rework recursive guard to keep array2string signature #9688

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

Merged
merged 1 commit into from
Sep 26, 2017

Conversation

ahaldane
Copy link
Member

@ahaldane ahaldane commented Sep 14, 2017

#8963 caused the signature of np.array2string to be mangled in 1.13 because of the decorator wrapping. Currently the signature is np.array2string(self, *args, **kwargs) when it should be like np.array2string(a, max_line_width=None, precision=None, suppress_small=None, separator=' ', prefix='', style=<built-in function repr>, formatter=None).

This is a simple rework of that solution which maintains the array2string signature, so it is inspectable by users. For instance, I usually view signatures in ipython by typing a ? after a function name.

I haven't added a test since there is no functionality change. Conceivably I could add a test using the inspect module, but I'm not sure whether we should start testing signatures.

(It might be worth a separate PR to add signature change tests for many numpy functions).

(This PR is split off from #9139.)

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@eric-wieser
Copy link
Member

eric-wieser commented Sep 14, 2017

Note that this mangling only occurs in python 2 - any test using signature would pass in python 3, since functools.wraps copies __signature__.

If we actually care about py2 signatures, then every use of functools.wraps in the codebase is "wrong".

One important question is what version of python the docs are built with - clearly it ought to be py3 if some of our signatures are missing

@charris
Copy link
Member

charris commented Sep 17, 2017

@eric-wieser @rgommers Ralf, what version of Python do you build the docs with?

@rgommers
Copy link
Member

Ralf, what version of Python do you build the docs with?

2.7. But it should work with all supported Python versions

@ahaldane
Copy link
Member Author

ahaldane commented Sep 17, 2017

Re: whether other decorators are broken in numpy

I only see a single other use of wraps in numpy, which was recently added in #8996, which in that case gives a pretty sensible "new" signature which I don't think needs to change.

Also, the signatures break in python 3 for py <= 3.3 too. (Though I doubt many people use those versions any more).

@eric-wieser
Copy link
Member

As far as I can tell from setup.py, it seems that numpy is not targeting 3.3 any more, so that doesn't actually matter.

But yeah, if this is the only place we run into this problem, I think you're right to fix this on all python versions by removing the decorator.


return decorating_function
_array2string_running = set() # used for recusion guard, see in array2string
Copy link
Member

Choose a reason for hiding this comment

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

Typo in recusion

@ahaldane
Copy link
Member Author

typo fixed

@charris
Copy link
Member

charris commented Sep 25, 2017

@eric-wieser Good to go?

@eric-wieser
Copy link
Member

eric-wieser commented Sep 25, 2017

Tempted to say we should use a context manager instead here, so that it can still be factored out:

@contextlib.contextlibmanager
def repr_guard(func, obj, repr_running={}):
    key = func, id(obj), get_ident()
    repeated = key in repr_running
    repr_running.add(key)
    try:
        yield repeated
    finally:
        repr_running.discard(key)
def array2string(a, ...):
    with repr_guard(array2string, a) as recursed:
        if rescursed:
            return '...'
        do_the_normal_stuff

@ahaldane
Copy link
Member Author

Seems like potentially a premature generalization, but maybe that's just my bias - I like to avoid abstractions if there's no code duplication.

Is there anywhere else in numpy we might want to use this?

@eric-wieser
Copy link
Member

eric-wieser commented Sep 25, 2017

Nope, this is probably not going to be used anywhere else. I tend to be biased the other way, preferring abstractions even if they only get used once. It is a bit of a clumsy generalization though, since decorators weren't an option.

Since this is just a matter of opinion, I'll just go ahead and merge what you have.

@eric-wieser
Copy link
Member

Wait, actually there's a much easier fix here - keep the decorator, but move it to the internal _array2string, which doesn't need to preserve its signature

@ahaldane
Copy link
Member Author

Hmm that should work.. I'll give it a try.

@ahaldane
Copy link
Member Author

Done, I'm happy doing as you said. So this PR just comes down to moving _array2string.

next_line_prefix, separator,
_summaryEdgeItems, summary_insert)[:-1]
return lst

Copy link
Member

@eric-wieser eric-wieser Sep 25, 2017

Choose a reason for hiding this comment

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

Nit: PEP8 wants two newlines here (and above the function)

@eric-wieser eric-wieser merged commit ceef499 into numpy:master Sep 26, 2017
@eric-wieser
Copy link
Member

Thanks!

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.

5 participants