-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
Conversation
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.
Looks good to me.
Note that this mangling only occurs in python 2 - any test using If we actually care about py2 signatures, then every use of 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 |
@eric-wieser @rgommers Ralf, what version of Python do you build the docs with? |
2.7. But it should work with all supported Python versions |
Re: whether other decorators are broken in numpy I only see a single other use of Also, the signatures break in python 3 for py <= 3.3 too. (Though I doubt many people use those versions any more). |
As far as I can tell from 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. |
numpy/core/arrayprint.py
Outdated
|
||
return decorating_function | ||
_array2string_running = set() # used for recusion guard, see in array2string |
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.
Typo in recusion
b0ddf4b
to
d0d0857
Compare
typo fixed |
@eric-wieser Good to go? |
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 |
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? |
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. |
Wait, actually there's a much easier fix here - keep the decorator, but move it to the internal |
Hmm that should work.. I'll give it a try. |
d0d0857
to
afa2c39
Compare
Done, I'm happy doing as you said. So this PR just comes down to moving |
next_line_prefix, separator, | ||
_summaryEdgeItems, summary_insert)[:-1] | ||
return lst | ||
|
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.
Nit: PEP8 wants two newlines here (and above the function)
afa2c39
to
2ecae5e
Compare
Thanks! |
#8963 caused the signature of
np.array2string
to be mangled in 1.13 because of the decorator wrapping. Currently the signature isnp.array2string(self, *args, **kwargs)
when it should be likenp.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.)