Skip to content

MAINT: test, refactor design of recursive closures #11910

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 3 commits into from
Sep 17, 2018

Conversation

mattip
Copy link
Member

@mattip mattip commented Sep 8, 2018

In looking at flake8 --select F811 local variable reuse, I stumbled across the solution in PR #10621 to the problem of recursive closures (#10620). It seems we can avoid assigning the closure to None by using the design pattern in the last comment to that PR.

I also added a test for the use of recursive closures in loadtxt, which exposed another place where we still had cyclical references.

@mattip mattip force-pushed the recursive-closures branch from f9c724e to 75de762 Compare September 9, 2018 05:05
@eric-wieser
Copy link
Member

eric-wieser commented Sep 11, 2018

I'm not sure this is an improvement for clarity, although good job finding the other cases. Thoughts, @ahaldane?

Relates to https://bugs.python.org/issue4921, for anyone reading this who hasn't already found that.

@eric-wieser
Copy link
Member

A possible alternative:

class recursive(object):
    def __init__(self, func):
        self.func = func
    def __call__(self, *args, **kwargs):
        return self.func(self, *args, **kwargs)

used as

@recursive
def block_recursion(self, arrays, depth=0):
    ...
    self(arrays[i], ...)

block_recursion(arrays)

@eric-wieser
Copy link
Member

eric-wieser commented Sep 11, 2018

Or even simpler, but more cryptically:

def recursive(f):
    return f.__get__(f)

used the same way as above

Edit: This doesn't work

@@ -808,3 +808,11 @@ def _is_from_ctypes(obj):
return 'ctypes' in ctype_base.__module__
except Exception:
return False


class recursive(object):
Copy link
Member

Choose a reason for hiding this comment

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

This could do with a comment about why it is needed

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks like an improvement to me. Let's wait for one more opinion, since @recursive was my idea and might be an opinionated one.

@ahaldane
Copy link
Member

I like this much more than the magical = Nones from before. Merging is good by me!

Perhaps a slight quibble is that I might call it something more specific, like recursive_closure, but feel free to ignore me on that.


'''
def __init__(self, func):
self.func = func
Copy link
Member

Choose a reason for hiding this comment

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

nit: this should probably call functools.update_wrapper

@eric-wieser eric-wieser merged commit 73d7871 into numpy:master Sep 17, 2018
@eric-wieser
Copy link
Member

Whatever, there's little point updating the wrapper if its a local variable anyway. Thanks @mattip!

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.

4 participants