Skip to content

scrub kwarg and use one figure for all of the tests #56

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

Closed
wants to merge 6 commits into from

Conversation

ivanov
Copy link
Member

@ivanov ivanov commented Mar 24, 2011

The changes here fix a substantial memory leak in the test suite which was caused by generating a new figure for every test but never closing any of them. The memory usage for the test suite went from being monotonically increasing ramp that goes up to ~800MB with each test, to being a much more reasonable ~100MB.

To implement this, all of the tests which created new figures now just use one particular figure -- figure(1) -- which is also cleared just before the test.

Previously, clearing a figure removed all of the artists but kept around the state of the subplotpars, which caused problems for all tests that ran after a change to subplot parameters was made in the test suite, so I added a 'scrub' keyword argument to clf to also reset the subplot parameters, and also added a rcParam['figure.autoscrub'] to control the behavior of when clf is called without explicitly specifying 'scrub'.

These new tests were documented in both the CHANGELOG and in docs/api/api_changes, and the docs built fine (with all of the links and format working as intended)

ok, i'm closing this one in favor of #96

@WeatherGod
Copy link
Member

All my computers thank you greatly (especially my poor EeePC). I haven't tested this yet, though, but this has been a thorn in my side for a long time. Tests take a long time for me to run because my computer starts thrashing the swap-space, and then cleaning out the virtual memory at the end of the tests takes a long time.

Thanks!

subplotparameters to the defaults.

If *scrub* is None, the behavior it will be set to
rcParams['figure.autoscrub'].
Copy link
Member

Choose a reason for hiding this comment

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

IMHO the rc parameter is overkill. We could default to False and let users who like the scrubbing clear define an alias def scrub(): clf(scrub=True), or we could even define it for them.

Copy link
Member Author

Choose a reason for hiding this comment

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

the reason I went with the kwarg is because I think users would have a reasonable expectation to have the clear figure command to clear the figure completely, as opposed to having to call a separate command for doing that. But given that others might want to keep the old behavior, we can leave as a user configurable option. The new default behavior (figure.autoscrub = True) allows for the plt.clf() commands to stay as is in the rest of the pull request (though I realize you don't like the code duplication).

@ivanov
Copy link
Member Author

ivanov commented Apr 21, 2011

any objections to merging this?

@ivanov ivanov closed this Apr 21, 2011
@ivanov ivanov reopened this Apr 22, 2011
@ivanov
Copy link
Member Author

ivanov commented Apr 22, 2011

apologies, i pushed the comment and close button instead of simply comment

@efiring
Copy link
Member

efiring commented Apr 24, 2011

Paul, I think this has gotten too complicated, so I don't recommend merging this changeset. Instead, I recommend that your close-fig branch, or some variation of it, be applied to v1.0.x and merged into master ASAP. Failing to close the figures in the tests is a real bug, and should be fixed in the maintenance branch. There is another major bug in testing in master that I would like to fix, but to avoid more complicated merges, I would like to see the close patch applied first.
Resetting the subplot parameters is a separate issue, and should not get scrambled up with the basic bug fix.
Regarding that, I agree with Jouni that an rcParam is overkill; the rc system should be used sparingly. I get the impression that all we need may be a figure method that resets subplot params.
The value of reusing a figure in testing sounds marginal at best; but if it is to be done, it should be a separate changeset in master.

@efiring
Copy link
Member

efiring commented Apr 24, 2011

More miscellaneous comments:
It is not at all clear to me that a user would expect or want clf to reset the subplot params; and if a user did, it would not be obvious that that is what a kwarg called "scrub" is for. My guess is that in most cases where a user really wants to reuse a figure, the user will not want to have the subplot params reset. Better to leave clf() simple, let it just continue to delete artists. If one really wants a clean slate, one can start with a new figure--and even then one has to be careful because there are other ways that global state can be altered. Not just via the rc system, but via units support, and probably other ways I haven't thought of. (The units support is currently fouling up a subsequent test; this is what I want to get fixed as soon as the close/clf issue is resolved.)
The reason close() is slow is that it is calling gc.collect() to ensure cyclic references are cleaned up immediately. Maybe this is overkill, but I suspect it is better left in place than removed or made optional. This might be worth some investigation.
We can probably save 30-40 seconds on the tests merely by modifying the hexbin test; again, I want to do that after close/clf is resolved.
I suspect it would also be possible to speed up the tests a bit by modifying the image_comparison decorator so that it takes a function returning a figure, and then uses the savefig method on that figure to generate the various file types. That would cut the number of figure generations down by a factor of 3. That decorator is horrendously complicated, though.

@mdboom
Copy link
Member

mdboom commented Apr 25, 2011

After thinking on this for a while, I think I agree. I think fixing the
memory consumption in the tests is important, but the public API should be
driven by the needs of general usage. In that latter context, the "scrub"
kwarg doesn't seem terribly useful.

But let's get all those figures closing in the tests come hell or high water
-- the difference is staggering!

On Sun, Apr 24, 2011 at 7:10 PM, efiring <
reply@reply.github.com>wrote:

I suspect it would also be possible to speed up the tests a bit by
modifying the image_comparison decorator so that it takes a function
returning a figure, and then uses the savefig method on that figure to
generate the various file types. That would cut the number of figure
generations down by a factor of 3. That decorator is horrendously
complicated, though.

That's a good idea.

@mdboom
Copy link
Member

mdboom commented Apr 26, 2011

I have some time today -- I'm going to look into cleaning up that decorator and see if we can save the figure regeneration time -- it's also a good place to automatically close figures when done which probably addresses the problem this pull request addresses.

@ivanov
Copy link
Member Author

ivanov commented Apr 26, 2011

ok, i'm closing this one in favor of #96

@ivanov ivanov closed this Apr 26, 2011
@richbwood richbwood mentioned this pull request Dec 19, 2012
fariza added a commit to fariza/matplotlib that referenced this pull request Sep 22, 2016
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.

5 participants