Skip to content

Creation of the 'classic' matplotlib style#4669

Merged
tacaswell merged 11 commits into
matplotlib:masterfrom
WeatherGod:classic_mode
Aug 14, 2015
Merged

Creation of the 'classic' matplotlib style#4669
tacaswell merged 11 commits into
matplotlib:masterfrom
WeatherGod:classic_mode

Conversation

@WeatherGod

Copy link
Copy Markdown
Member

Dependent upon #4647. Also not fully tested and vetted. I need to see if I missed any params.

@WeatherGod

Copy link
Copy Markdown
Member Author

also still needs docs.

@WeatherGod WeatherGod added this to the next point release milestone Jul 12, 2015
@WeatherGod

Copy link
Copy Markdown
Member Author

sortof blocked by #4218

@WeatherGod

Copy link
Copy Markdown
Member Author

This PR is now based on the other validation PR. I have also updated this PR to base all unit tests on the new 'classic' mode. Also added a special 'default' style.

@WeatherGod

Copy link
Copy Markdown
Member Author

Ok, managed to get a few more tests passing (apparently, some of them were very sensitive to path simplification thresholds of 0.1 versus 0.111111111).

I have found a bug with the image comparison class that I exacerbated. If the test modifies the rc params as part of the test, and those rc params do not take effect until render time (i.e., path simpliciation, dpi, etc.), then the context manager in which we establish the default style on all tests nukes those changes since the savefig step happens outside the context manager. I am not sure what to do about that.

@efiring

efiring commented Jul 14, 2015

Copy link
Copy Markdown
Member

@WeatherGod, I think you have hit upon an issue that has been worrying me with respect to the plans to redo the whole rcParams/configuration/property/traitlets business: there are quite a few places where some things are now set at object creation time, others at draw time; where things get modified based on initial drawing; etc. The possibilities for breakage and running into gotchas are extensive. This is going to be hard to sort out.

@WeatherGod

Copy link
Copy Markdown
Member Author

@efiring, this is very true, and I have no delusions that the whole traitlets/serialization business will be easy. But if matplotlib is to grow, we need a better system to build it upon, so I would rather take the pain now.

As for the problem at hand, perhaps the solution is fairly simple... don't use a context manager. Instead, I should probably hook onto the setup/cleanup mechanisms and manage the style handling that way.

@WeatherGod

Copy link
Copy Markdown
Member Author

Making progress with this approach. Can anyone tell me why I am getting errors trying to find the font family 'sans-serif'? I have also tried 'sans-serif' to no avail.

@WeatherGod

Copy link
Copy Markdown
Member Author

Well, looks like everything passes on Travis, even though it fails miserably locally for me. That's not exactly warm fuzzies for me...

I'll work on adding documentation tonight.

@WeatherGod

Copy link
Copy Markdown
Member Author

Just did a rebase and incorporated the recent feature to specify inverse functions for deprecated rcparams. I'll update later for any new rc params and write up docs.

@WeatherGod

Copy link
Copy Markdown
Member Author

...and I am getting my PRs mixed up. The previous comment was meant for my property-cycler PR. This one has also been rebased, but needs the new params and documentation.

@WeatherGod

Copy link
Copy Markdown
Member Author

huh, now a lot of stuff fails on Travis. A bunch of it relating to tight_layout and others related to stix fonts. I suspect the tight_layout problems really are font problems in disguise. Maybe I have my font specifications all wrong in the classic style?

@WeatherGod

Copy link
Copy Markdown
Member Author

I am getting the following warning in Travis:

/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/matplotlib-1.5.dev1-py2.7-linux-x86_64.egg/matplotlib/font_manager.py:1285: UserWarning: findfont: Font family [u'Bitstream Vera Sans'] not found. Falling back to Helvetica
  (prop.get_family(), self.defaultFamily[fontext]))

This is probably the source of my font problems. But I am not clear how my 'classic' style is any different from the defaults.

@mdboom

mdboom commented Jul 28, 2015

Copy link
Copy Markdown
Member

I think the message:

/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/matplotlib-1.5.dev1-py2.7-linux-x86_64.egg/matplotlib/font_manager.py:1285: UserWarning: findfont: Font family [u'Bitstream Vera Sans'] not found. Falling back to Helvetica
  (prop.get_family(), self.defaultFamily[fontext]))

is a red herring. It's emitted on master as well, and I think has to do with a test writing explicitly to the PostScript backend with embedded fonts only turned on. (I should probably be in a catch_warnings block since it's in fact benign in this case).

I'll push your branch to a branch on the main fork so that we can get a downloadable image packet.

@mdboom

mdboom commented Jul 28, 2015

Copy link
Copy Markdown
Member

Here's the Travis build for the temporary branch: https://travis-ci.org/matplotlib/matplotlib/builds/73086282

@WeatherGod

Copy link
Copy Markdown
Member Author

Thanks @mdboom. I'll take a good look at the results tomorrow.

In the meantime, I just added some preliminary documentation about this feature in "whats new". I noticed that it doesn't seem like any of the other new styles are mentioned. It got me thinking that it might be nice to have a page that showcases the available styles that matplotlib ships with?

@tacaswell

Copy link
Copy Markdown
Member

@tonysyu has some code for making a style gallery someplace.

@mdboom

mdboom commented Jul 28, 2015

Copy link
Copy Markdown
Member

And the result images (from the Python 3.4 build, if that matters) are now available here:

https://s3.amazonaws.com/matplotlib-test-results/artifacts/7686/7686.4/result_images.tar.bz2

@mdboom

mdboom commented Jul 28, 2015

Copy link
Copy Markdown
Member

Looking at the images:

For the SVG ones, both the baseline and generated images are using Bitstream Vera Sans, but there does seem to be some moving around of objects everywhere.

For the PNG ones, the baseline images seem to have Helvetica (or maybe Arial), whereas the generated ones are Bitstream Vera Sans. So this suggests to me that something is really funky with the font lookup and maybe the baseline images were generated with the wrong fonts. I'll see if I can reproduce the failure locally to confirm.

@WeatherGod

Copy link
Copy Markdown
Member Author

Well, more specifically, this is happening with my classic mode stuff,
so... for some reason there is a difference between specifying the fonts
via an rc file, and not, even though the resulting rcParams object is
(effectively) the same.

On Tue, Jul 28, 2015 at 5:33 PM, Michael Droettboom <
notifications@github.com> wrote:

Looking at the images:

For the SVG ones, both the baseline and generated images are using
Bitstream Vera Sans, but there does seem to be some moving around of
objects everywhere.

For the PNG ones, the baseline images seem to have Helvetica (or maybe
Arial), whereas the generated ones are Bitstream Vera Sans. So this
suggests to me that something is really funky with the font lookup and
maybe the baseline images were generated with the wrong fonts. I'll see if
I can reproduce the failure locally to confirm.


Reply to this email directly or view it on GitHub
#4669 (comment)
.

@WeatherGod

Copy link
Copy Markdown
Member Author

Ok, now trying to get down and dirty to make sure there aren't unnoticed differences:

python -c "import matplotlib; print matplotlib.rcParamsDefault" > mpl_defaults.txt
python -c "import matplotlib; print matplotlib.RcParams(**matplotlib.rcParamsOrig)" > mpl_origs.txt
python -c "import matplotlib.style; print matplotlib.rc_params_from_file('lib/matplotlib/mpl-data/stylelib/classic.mplstyle', False, False)" > mpl_classic.txt

When I diff mpl_defaults.txt with mpl_origs.txt, I just get a difference in default backends, which is expected.

When I diff mpl_defaults.txt with mpl_classic.txt, I get the following:

42,45d41
< backend: agg
< backend.qt4: PyQt4
< backend.qt5: PyQt5
< backend_fallback: True
80,81d75
< datapath: /nas/home/broot/centos6/lib/python2.7/site-packages/matplotlib-1.5.dev1-py2.7-linux-x86_64.egg/matplotlib/mpl-data
< docstring.hardcopy: False
83d76
< examples.directory: 
121d113
< interactive: False
179c171
< path.effects: []
---
> path.effects: 
192d183
< plugins.directory: .matplotlib_plugins
222d212
< timezone: UTC
227,229d216
< webagg.open_in_browser: True
< webagg.port: 8988
< webagg.port_retries: 50

So, it looks like I need to correct the path.effects parameter, but I think all the others are safe to exclude from the classic mode file.

@mdboom

mdboom commented Aug 6, 2015

Copy link
Copy Markdown
Member

@WeatherGod: I'm sort of driveby commenting here, so ignore me if I'm out of date. Are we still having font match issues here? You may want to try #4689.

@WeatherGod

Copy link
Copy Markdown
Member Author

I was just thinking that as I was reading those comments. I'll give it a
shot tonight on my Ubuntu machine and see if it resolves it.

On Thu, Aug 6, 2015 at 4:54 PM, Michael Droettboom <notifications@github.com

wrote:

@WeatherGod https://github.com/WeatherGod: I'm sort of driveby
commenting here, so ignore me if I'm out of date. Are we still having font
match issues here? You may want to try #4689
#4689.


Reply to this email directly or view it on GitHub
#4669 (comment)
.

@WeatherGod

Copy link
Copy Markdown
Member Author

sigh... does not appear to be the case.

@WeatherGod

Copy link
Copy Markdown
Member Author

Interesting observation... If I change the default style used in the testing decorator to be 'default', then I get the same failures. So the problem isn't caused by the classic mode, per-se. If I take out the style-handling in the setup/teardown code, then things are back to normal (but classic mode is never applied for the tests).

@WeatherGod

Copy link
Copy Markdown
Member Author

Curiouser and curiouser... if I don't use "matplotlib.rcParams.update()" at all for the tests, and run with --verbose-debug, I get a single findfont message:

findfont: Matching :family=sans-serif:style=normal:variant=normal:weight=400:stretch=normal:size=medium to Bitstream Vera Sans (u'/home/broot/scratch/miniconda/lib/python2.7/site-packages/matplotlib/mpl-data/fonts/ttf/Vera.ttf') with score of 0.000000

But, if I use "matplotlib.rcParams.update()", then I get two findfont messages, the first one the same as above, and the second one slightly different:

findfont: Matching :family=sans-serif:style=normal:variant=normal:weight=400:stretch=normal:size=medium to Bitstream Vera Sans (u'/home/broot/scratch/miniconda/lib/python2.7/site-packages/matplotlib/mpl-data/fonts/ttf/Vera.ttf') with score of 0.000000
.findfont: Matching :family=Bitstream Vera Sans:style=normal:variant=normal:weight=400:stretch=normal:size=medium to Bitstream Vera Sans (u'/home/broot/scratch/miniconda/lib/python2.7/site-packages/matplotlib/mpl-data/fonts/ttf/Vera.ttf') with score of 0.000000

@WeatherGod

Copy link
Copy Markdown
Member Author

and I can no longer reproduce that double-findfont behavior... This is driving me insane!

I can't be 100% sure that this is a font problem, per-se. Looking closely at the test failure for mplot3d's text3d test, the z-axis line is not drawn quite right (it is very close to vertical, but not quite).

@WeatherGod

Copy link
Copy Markdown
Member Author

So now I tried running the text3d test completely separately from any test framework and generate an image from that (no applying of any particular styles). Now, since I have been able to run the tests in such a way that they pass (by completely turning off the apply/restore of styles for each test), I would expect that this image be identical to the baseline image, but it isn't! It was identical to the failed image!

This makes me wonder if there is something "wrong" with the testing framework that is applying some sort of setting that is carrying over from one test to the next, and that my patch "fixes" that? Note that test_mplot3d.py does not set any rcparams, so it isn't the tests themselves that is causing the problem (I don't think).

Perhaps we have a wayward rcparam assignment somewhere in our codebase, and I am undoing it at the end of each test?

@WeatherGod

Copy link
Copy Markdown
Member Author

Aha! There is code somewhere that is modifying the rcparams. I did the following to the teardown function:

    def teardown_class(cls):
        CleanupTest.teardown_class()
        try:
            assert_dict_equal(mpl.rcParams, cls._initial_settings)
        except AssertionError as e:
            with open('tempy.txt', 'w') as fh:
                p1 = mpl.rcParams
                p2 = cls._initial_settings
                if set(p1.keys()) != set(p2.keys()):
                    diffs = set(p1.keys()) - set(p2.keys())
                    fh.write("Different keys: %s\n" % diffs)
                for k in p2.keys():
                    v1 = p1[k]
                    v2 = p2[k]
                    if v1 != v2:
                        fh.write("Diff values for %s: %s  %s\n" % (k, v1, v2))
            raise

(done this way because nose kept truncating the assertion's message). I got the following result upon teardown of the second test for test_mplot3d (and after the third test for test_axes_grid1.py):

Diff values for font.family: [u'Bitstream Vera Sans']  [u'sans-serif']
Diff values for backend: agg  TkAgg
Diff values for text.hinting: False  auto

So, somehow, text hinting is getting turned off, and it stays off for the rest of the tests. The font.family being switched to "Bitstream Vera Sans" is probably what is throwing off the stix tests, I'd imagine.

@WeatherGod

Copy link
Copy Markdown
Member Author

And I have come around to the source of the issue.

In matplotlib.tests, there is a function called setup(), that explicitly modifies the font family, text hinting, and the text hinting factor. This function is called in matplotlib.testing.decorators._do_cleanup() and in matplotlib.testing.decorators.switch_backend(). Still not entirely sure why the change in the params only happens after a few tests (I am guessing that it is the backend switching that is causing it?). I am going to try re-arranging the setup and teardown code a bit.

@WeatherGod

Copy link
Copy Markdown
Member Author

Looks like all I need to do is move that call to matplotlib.tests.setup() out of _do_cleanup() and into the CleanupTests.setupClass() class method (which seemed like the more logical place for it, anyway). It seems to allow mplot3d and axes_grid1 tests to pass. Now trying for the entire suite.

@WeatherGod

Copy link
Copy Markdown
Member Author

Close, but no cigar. We need to do param cleanup in the cleanup decorator as well.

By the way, I think I have found the source of the bug that caused the intermittent latex failures! There is likely a race condition because the testing is being done in two threads, and the tests related to the latex failures heavily depend upon modifying the rcparams on-the-fly. In particular, setting "usetex" to True. Well, if "usetex" gets unset prior to calling the savefig for that test, then latex wouldn't get called, and the stringio comparison function would just sit there, waiting for something that will never come.

@tacaswell

Copy link
Copy Markdown
Member

I thought the tests ran as separate processes, not threads.

@WeatherGod

Copy link
Copy Markdown
Member Author

That's what I thought, and maybe I am wrong. It is something to keep in mind for further investigation.

@WeatherGod

Copy link
Copy Markdown
Member Author

Woot! The tests pass! Someone should probably take a peek at the bastardization I did to the testing framework, but it seems like it works!

tacaswell added a commit that referenced this pull request Aug 14, 2015
ENH: Creation of the 'classic' matplotlib style
@tacaswell tacaswell merged commit caedbca into matplotlib:master Aug 14, 2015
@mdboom

mdboom commented Aug 20, 2015

Copy link
Copy Markdown
Member

👏

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.

4 participants