Repository navigation
Make "classic" style have effect - #5437
WeatherGod merged 3 commits into
Conversation
Sorry, something went wrong.
|
@mdboom Did this get folded into something else? |
Sorry, something went wrong.
|
No -- I was testing it in conjuction with #5416, but I think we can/should still merge this as a separate PR first. |
Sorry, something went wrong.
|
It's just the symlog image comparison test that is failing now. Not sure why yet. |
Sorry, something went wrong.
|
I think this is ready to go in now. We should probably merge sooner than later to prevent fewer false positive results from creeping in. |
Sorry, something went wrong.
There was a problem hiding this comment.
Tab this over one so that it doesn't line up with the code block.
Sorry, made that comment in the commit rather than the PR.
Sorry, something went wrong.
|
I haven't personally vetted the conversion from dictionaries to classes, but the unit tests don't fail, so I am guessing that is ok. Besides the one little PEP8 thing, I am +1 (and even that, I don't care too much about). I am a bit curious why the pep8 tests didn't catch that. |
Sorry, something went wrong.
|
Pretty sure that is one of the pep8 errors/warnings we have blacklisted. |
Sorry, something went wrong.
|
Really? That is actually one of the PEP8 rules that I think makes the most On Mon, Nov 16, 2015 at 10:51 AM, Thomas A Caswell <notifications@github.com
|
Sorry, something went wrong.
|
👍 It looks like some of the symlog images are changed twice. Would it make sense to squash the commits? |
Sorry, something went wrong.
|
Sure, I can squash the commits. Also, let's hold off -- I think I just discovered another case where this isn't working... |
Sorry, something went wrong.
Fix failures in matplotlib#5416
|
Ok -- I've fixed one last thing and now I have my massive style changes branch passing (stay tuned). (The @cleanup decorator wasn't applying the classic style, which soon will affect things like view limits that even non-image-comparison tests care about). Also addressed @WeatherGod's indentation comment. Once Travis passes, I'd say this is good to go. |
Sorry, something went wrong.
There was a problem hiding this comment.
isn't this backwards?
Sorry, something went wrong.
There was a problem hiding this comment.
ok, nevermind... I got mixed up
Sorry, something went wrong.
There was a problem hiding this comment.
No. The idea here is to keep @cleanup without arguments working. If you pass it no arguments, the first argument should be a function and we want to use classic mode by default. If you pass a string it specifies a style, and the function will be passed in later (through function chaining). This is just typical decorator madness, but I can add a comment if you think it would help.
Sorry, something went wrong.
There was a problem hiding this comment.
Perhaps make a comment here explaining that style in some cases isn't really a style name?
Sorry, something went wrong.
There was a problem hiding this comment.
I've added a comment.
Sorry, something went wrong.
|
one of the Travis errors seems spurious, but the other one seems to be some sort of odd stix font issue that only happens in py3.4? I restarted the spurious one, though. |
Sorry, something went wrong.
Travis looks green now. |
Sorry, something went wrong.
There was a problem hiding this comment.
Perfect! Very clear and helpful.
Sorry, something went wrong.
Make "classic" style have effect
|
I presume this gets cherry-picked to v2.0.x branch, or did we want to hold off on that while other stuff is going on? |
Sorry, something went wrong.
|
No, it should be on 2.0.x now -- everything else we're doing ends up sort of relying on this. I'll go ahead and do that now. |
Sorry, something went wrong.
Make "classic" style have effect
Hopefully fixes failures in #5416
EDIT: I'm adding some background to these changes up here.
We discovered in #5416 that the code to force classic mode on all of the tests by default was broken, because the classic style was being turned on but later the rcParams were all being reset to defaults. So it was a matter of changing the order of operations to
(1) Reset to rcdefaults
(2) Apply the classic style
(3) Additionally apply some settings we've always done for testing such as turning font hinting off
The next step is that in #5214, when the default font was changed from Bitstream Vera to Deja Vu, the classic style was still left at Vera. This means that the classic style was using fonts that we don't ship, which is a problem on Travis, but also could be a problem for users who may not have Vera installed elsewhere on their system. Since these two fonts are identical in looks, and DejaVu only adds additional characters (and some "bugfixes") I think it's still within the definition of "classic mode" to move to the new font.
#5214 also accidentally changed a baseline image to use the new default math font rather than the classic computer modern math font. (Probably just based on the assumption that classic mode testing was working). This has been reverted.
The last thing is really obscure. There is code in mathtext that controls the placement of sub/superscripts based on the current mathtext.fontset rcParam. Each of the fontsets, (cm, stix* and dejavu*) need slightly different offsets for the sub/superscripts and that information is not stored in the font file itself. However, it's actually possible to use DejaVu font when mathtext.fontset is "cm" when the text is inside a \mathdefault{} block (which means use the non-math font for the content inside {}). All that means that when in classic mode, the exponents in the ticks on a log plot were misplaced. The fix is to use the sub/superscript offsets appropriate for the "current font in use within a particular part of the math expression" rather than the global rcParam "mathtext.fontset".
All that hopefully means we properly have classic mode working in all tests and we can proceed with the style changes, but Travis-CI will tell.