GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Faster image comparison decorator by mdboom · Pull Request #97 · matplotlib/matplotlib

Conversation

mdboom commented Apr 27, 2011

Copy link

Copy Markdown

Member

This is a riff on the ideas in the comments on the pull requests by Paul @ivanov to reduce memory consumption when running the regression tests. See:

#96

and

#56

This is different from the other two in that it:

Calls the test code to generate the figure object only once, and then calls savefig three times on the same figure -- this results in an ~ 12% speedup.
Automatically closes all figures and restores rcParams and the unit registry after every test. The latter prevents some nasty side effect problems presented by some tests. This also revealed a memory leak in pyplot.close('all'). This saves massive amounts of memory vs. the current master, but it roughly equivalent to Paul's approach in pull request 96.
Runs Inkscape in "shell" mode so that a single inkscape process is fired up and re-used to convert each SVG to PNG. This results in an additional ~ 32% speedup.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mdboom added 3 commits

April 26, 2011 16:29


          Updates to the image comparison testing decorator:

27d4c0c

  1) Run the test itself that generates figure(s) only once, then
     savefig it to each output format.  This appears to have an approx
     2.75x speedup on run time.

  2) Cleanup better after each test:

       Close all figures
       Reset rcParams to their defaults
       Reset the unit registry to defaults

This requires the following changes to the tests themselves:

  1) The tests no longer need to call 'savefig' -- just create figures
     and the decorator will find them.

  2) Tests that don't use the image_comparison decorator should use the
     new cleanup decorator to make sure that cleanup happens.


          Fix memory leak in pyplot.close('all')

354dee8


          Run inkscape for SVG conversion as a server -- prevents starting inks…

a074823

…cape up multiple times.

mdboom commented Apr 27, 2011

Copy link

Copy Markdown

Member Author

I also want to point out a cool script I found for measuring peak memory usage of a process on Linux:

https://gist.github.com/526585

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

WeatherGod commented Apr 27, 2011

Copy link

Copy Markdown

Member

Quick note, on my computer, the Inkscape process for one of the conversions (I forget which) eats up a lot of memory (several hundreds). In the current form, at least this memory gets freed after the conversion is done. Will the memory get cleared after each conversion by Inkscape?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdboom commented Apr 27, 2011

Copy link

Copy Markdown

Member Author

I didn't detect any additional peak memory usage in Inkscape this way vs. the old way. Googling reveals various bug reports about Inkscape in --shell mode, but those may be obsolete bugs. I think this requires some testing with various Inkscapes on various systems to ensure it's an ok approach.

(I'm running Inkscape 0.48.1 on Fedora 14, FWIW)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ivanov reviewed

Apr 27, 2011

View reviewed changes

Comment thread

lib/matplotlib/testing/decorators.py

ivanov Apr 27, 2011

Copy link

Copy Markdown

Member

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.

shouldn't this be rcParamsDefault - since test may be launched after the user starts up e.g. ipython with a profile which modifies the rcParams - making test incompatible?

also, is there a reason to not just do it in one line, instead of two?
cls.original_rcParams = matplotlib.rcParamsDefault.copy()

same for the orignal_units_registry below

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mdboom Apr 27, 2011

Copy link

Copy Markdown

Member Author

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.

Good points on both. I think I was confused by the restoration phase -- which does need the .clear and .update because we can't replace the underlying rcParams object which is referenced all over the matplotlib source code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mdboom Apr 27, 2011

Copy link

Copy Markdown

Member Author

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.

Ah -- on your first point: it turns out we can't use rcParamsDefault since the test infrastructure overrides certain things in a setup() function. See lib/matplotlib/tests/init.py.

def setup():
    use('Agg', warn=False) # use Agg backend for these tests

    # These settings *must* be hardcoded for running the comparison
    # tests and are not necessarily the default values as specified in
    # rcsetup.py
    rcdefaults() # Start with all defaults
    rcParams['font.family'] = 'Bitstream Vera Sans'
    rcParams['text.hinting'] = False

Maybe we should call this function from the teardown part of the decorator? Then everything gets reset to this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ivanov commented Apr 27, 2011

Copy link

Copy Markdown

Member

Great job, Michael - this looks like it was a lot more work than my duct-tape fix ;)

I like the DRYer elimination of savefig everywhere, since the name is already passed to the decorator.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ivanov merged this pull request into matplotlib:master

Apr 27, 2011

ivanov commented Apr 27, 2011

Copy link

Copy Markdown

Member

oh, damn it! not this again - i suck.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdboom commented Apr 27, 2011

Copy link

Copy Markdown

Member Author

No worries. These github guys have made development too easy! :) I just pushed a couple of commits to this branch that address some of the comments here, though -- hopefully there's an easy way to apply them, too.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ivanov commented Apr 27, 2011

Copy link

Copy Markdown

Member

in my defense, the behavior of the merge button changed only two days ago:
https://github.com/blog/843-the-merge-button

it used to only provide you with instructions on how to do the merge, not perform it for you.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdboom commented Apr 27, 2011

Copy link

Copy Markdown

Member Author

Yeah -- it's a somewhat misguided feature, IMHO. It doesn't give an opportunity to test the change -- and it no longer provides the really useful instructions on how to do it manually.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ivanov commented Apr 27, 2011

Copy link

Copy Markdown

Member

damn, looks like a current bug:
http://support.github.com/discussions/site/3568-reopen-broken-in-issues-for-pull-request

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdboom commented Apr 27, 2011

Copy link

Copy Markdown

Member Author

I was able to open a new pull request, though, so not so bad, excepting losing the existing comments. It's here:

#98

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

richbwood mentioned this pull request

Dec 19, 2012

test_pcolormesh hangs #1609

Closed

magnunor pushed a commit to magnunor/matplotlib that referenced this pull request

Dec 5, 2013


          Merge pull request matplotlib#97 from francisco-dlp/ENH_image_plotting

2a82de1

Enh image plotting

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.