8000 BUG: reference cycle in np.vectorize by mattip · Pull Request #11977 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: reference cycle in np.vectorize #11977

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.

Sign up for GitHub

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 5 commits into from
Jan 9, 2019
Merged

BUG: reference cycle in np.vectorize #11977

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0267264
BUG: use cyclic garbage collection semantics in np.frompyfunc
mattip Sep 18, 2018
f73d96d
MAINT: cleanup comments and harmless reference assignment
mattip Dec 4, 2018
a3b9f75
MAINT: fixes from review
mattip Dec 18, 2018
7474a71
MAINT: fix from review
mattip Jan 9, 2019
82419c0
MAINT: simplify test (seberg)
mattip Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
MAINT: fixes from review
  • Loading branch information
@mattip
mattip committed Dec 18, 2018
commit a3b9f757bc473efe5803e1a164db9d832c43c7c9
11 changes: 5 additions & 6 deletions numpy/core/src/umath/ufunc_object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5296,6 +5296,7 @@ PyUFunc_RegisterLoopForType(PyUFuncObject *ufunc,
static void
ufunc_dealloc(PyUFuncObject *ufunc)
{
PyObject_GC_UnTrack((PyObject *)ufunc);
PyArray_free(ufunc->core_num_dims);
PyArray_free(ufunc->core_dim_ixs);
PyArray_free(ufunc->core_offsets);
Expand All @@ -5307,7 +5308,6 @@ ufunc_dealloc(PyUFuncObject *ufunc)
Py_DECREF(ufunc->identity_value);
}
if (ufunc->obj != NULL) {
PyObject_GC_UnTrack((PyObject *)ufunc);
Py_DECREF(ufunc->obj);
}
PyObject_GC_Del(ufunc);
Expand All @@ -5320,12 +5320,11 @@ ufunc_repr(PyUFuncObject *ufunc)
}

static int
ufunc_traverse(PyObject *self, visitproc visit, void *arg)
ufunc_traverse(PyUFuncObject *self, visitproc visit, void *arg)
{
PyUFuncObject *ufunc = (PyUFuncObject*)self;
Py_VISIT(ufunc->obj);
if (ufunc->identity == PyUFunc_IdentityValue) {
Py_VISIT(ufunc->identity_value);
Py_VISIT(self->obj);
if (self->identity == PyUFunc_IdentityValue) {
Py_VISIT(self->identity_value);
}
return 0;
}
Expand Down
8 changes: 4 additions & 4 deletions nump 8000 y/lib/tests/test_function_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1505,11 +1505,11 @@ class A(object):
iters = 20

def bound(self, *args):
return np.int(0)
return 0

@staticmethod
def unbound(*args):
return np.int(0)
return 0

def npy_bound(self, a):
return types.MethodType(np.frompyfunc(self.bound, 1, 1), a)
Copy link
Member

Choose a reason for hiding this comment

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

I'm very confused by what exactly we're trying to test here.

Copy link
Member Author

Choose a reason for hiding this comment

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

self.bound is part of a reference cycle that requires a gc collection cycle to break. I added more clarification in the test docstring. Improvements welcome

Copy link
Member

Choose a reason for hiding this comment

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

The PR looks good to me, if nobody complains will merge in a bit probably. The test couldbe a bit more obvious to read, or maybe add a trivial one:

def func(a):
    return 0
func.vectorized = np.frompyfunc(func)

giving a minimal cycle. The other part would probably be simpler by making it a realistic use case:

class A(object):
    def __init__(self):
        # Wrap bound_method into a ufunc with `np.frompyfunc`. This creates
        # a reference cycle:
        #     self.method -> self   # since it is a bound method (knows "self")
        #     self -> self.wrapped_method  # Set here
        #     self.wrapped_method -> self.method  # Stores self.method to call it
        self.wrapped_method = np.frompyfunc(self.method)

    def method(self):
        return 0

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I guess that breaks the parameterization. Although, I do not think you need the types.MethodType if the just do the self.bound_vectorized = np.frompyfunc(self.bound, 1, 1) before returning it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not following. Where would I add that line?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I think what confuses me is that I believe you do not need this at all. You can simply remove the npy_bound and npy_unbound logic and just make it a.f = np.frompyfunc(func, 1, 1).

Copy link
Member Author

Choose a reason for hiding this comment

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

Adopted

Expand All @@ -1527,8 +1527,8 @@ def test_frompyfunc_leaks(self, func, npy_func, incr):
# exposed in gh-11867 as np.vectorized, but the problem stems from
# frompyfunc.
# class.attribute = np.frompyfunc(<method>) creates a
# reference cycle that requires a gc collection cycle to break
# (on CPython 3)
# reference cycle if <method> is a bound class method that requires a
# gc collection cycle to break (on CPython 3)
import gc

gc.disable()
Expand Down
0