Skip to content

fixes issue #2556 #2558

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

Jump to bottom
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

@tacaswell
fixes issue matplotlib#2556
1b81e9f 
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
pep8 fix
fcaf629
@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.

@tacaswell
renamed import
5e69629 
renamed imported matplotlib.collections from
`collections` -> `mcollections` to try and avoid clashing with
`collections` in python core.
@tacaswell
added test_quiver module
c312cca 
tests memory leaks reported in issue matplotlib#2556
@tacaswell
fixed memory leak in quiverkey
4cc466e 
closes matplotlib#2556
pelson
pelson reviewed Jan 14, 2014
View reviewed changes
lib/matplotlib/quiver.py
@@ -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
Merge pull request #2558 from tacaswell/quiver_memory_leak
3d297ff 
fixes issue #2556
@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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.3.x blocker
Development

Successfully merging this pull request may close these issues.
4 participants
@tacaswell @efiring @mdboom @pelson