Skip to content
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

Creation of the 'classic' matplotlib style #4669

Merged
merged 11 commits into from Aug 14, 2015

Conversation

WeatherGod
Copy link
Member

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

@WeatherGod
Copy link
Member Author

also still needs docs.

@WeatherGod WeatherGod added this to the next point release milestone Jul 12, 2015
@WeatherGod
Copy link
Member Author

sortof blocked by #4218

@WeatherGod
Copy link
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
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
Copy link
Member

efiring commented Jul 14, 2015

@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
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
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
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
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
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
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
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
Copy link
Member

mdboom commented Jul 28, 2015

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
Copy link
Member

mdboom commented Jul 28, 2015

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

@WeatherGod
Copy link
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
Member

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

@mdboom
Copy link
Member

mdboom commented Jul 28, 2015

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
Copy link
Member

mdboom commented Jul 28, 2015

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
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
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
Copy link
Member

mdboom commented Aug 6, 2015

@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
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
Member Author

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

@WeatherGod
Copy link
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
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
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
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
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
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
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
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
Member

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

@WeatherGod
Copy link
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
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
Copy link
Member

mdboom commented Aug 20, 2015

👏

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.

None yet

4 participants