Skip to content
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

fixes issue #2556 #2558

Merged
merged 8 commits into from Jan 27, 2014
Merged

fixes issue #2556 #2558

merged 8 commits into from Jan 27, 2014

Conversation

tacaswell
Copy link
Member

Fixes it in two ways:

  1. changes the closure of on_dpi to be over a weakref, not self
  2. cleans up the call back when the quiver is removed

This should be a reasonable test:

X,Y = meshgrid( arange(0,2*pi,.04),arange(0,2*pi,.04) )
U = cos(X)
V = sin(Y)

fig = figure()
ax = fig.add_axes([0.1,0.1,0.8,0.8])
Q = ax.quiver( U, V)
ttX = Q.X
print "ttx: ", sys.getrefcount(ttX)
print "Q:",  sys.getrefcount(Q)

Q.remove()
print "ttx: ", sys.getrefcount(ttX)
print "Q: ", sys.getrefcount(Q)

del Q
print "ttx: ", sys.getrefcount(ttX)
assert sys.getrefcount(ttX) == 2

This SO question was helpful: http://stackoverflow.com/questions/10874499/understanding-reference-count-of-class-variable

Fixes it in two ways:

1) changes the closure of `on_dpi` to be over a weakref, not `self`
2) cleans up the call back when the quiver is removed
@tacaswell
Copy link
Member Author

Unclear why pep8 failed on 2.x, but passed on 3.x

@tacaswell
Copy link
Member Author

@efiring Can you take a look at this?

The travis failures are unrelated (ERROR: matplotlib.tests.test_bbox_tight.test_bbox_inches_tight_suptile_legend.test in both cases).

@efiring
Copy link
Member

efiring commented Nov 18, 2013

@tacaswell, It looks like QuiverKey also needs this treatment. Is this correct?

@tacaswell
Copy link
Member Author

I believe so.

@efiring
Copy link
Member

efiring commented Nov 18, 2013

I haven't tested the actual changeset, but it looks like it would do the job. I'm not positive that both the weak reference and the callback cleanup are needed--are there circumstances when the latter would not be sufficient?

@tacaswell
Copy link
Member Author

If the user does not call qiv.remove() but reaches in and directly deletes the quiver object from Axes.artists then the call back will not be cleaned up.

Unless the weakref is actively harmful, I think both should be used.

@efiring
Copy link
Member

efiring commented Jan 13, 2014

@tacaswell Do you want to go ahead and apply the same treatment to QuiverKey before this is merged?

@tacaswell
Copy link
Member Author

If this looks good to you, then yes.

@efiring
Copy link
Member

efiring commented Jan 13, 2014

@tacaswell It does look OK to me. @mdboom, do you want to check it? You have much more experience with weakref sorts of things than I do. It would be nice to get this finished.

renamed imported matplotlib.collections from
`collections` -> `mcollections` to try and avoid clashing with
`collections` in python core.
tests memory leaks reported in issue matplotlib#2556
@@ -850,7 +888,7 @@ def __init__(self, ax, *args, **kw):

#Make a collection
barb_size = self._length ** 2 / 4 # Empirically determined
collections.PolyCollection.__init__(self, [], (barb_size,), offsets=xy,
mcollections.PolyCollection.__init__(self, [], (barb_size,), offsets=xy,
Copy link
Member

Choose a reason for hiding this comment

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

Line is too long now... (80 chars apparently)

mdboom added a commit that referenced this pull request Jan 27, 2014
@mdboom mdboom merged commit 3d297ff into matplotlib:v1.3.x Jan 27, 2014
@tacaswell tacaswell deleted the quiver_memory_leak branch February 8, 2014 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants