-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
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']. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
any objections to merging this? |
apologies, i pushed the comment and close button instead of simply comment |
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. |
More miscellaneous comments: |
After thinking on this for a while, I think I agree. I think fixing the But let's get all those figures closing in the tests come hell or high water On Sun, Apr 24, 2011 at 7:10 PM, efiring <
|
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. |
ok, i'm closing this one in favor of #96 |
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