Make "classic" style have effect #5437
Conversation
5dfecec to
5551cdd
Compare
|
@mdboom Did this get folded into something else? |
5551cdd to
71093fa
Compare
|
No -- I was testing it in conjuction with #5416, but I think we can/should still merge this as a separate PR first. |
|
It's just the symlog image comparison test that is failing now. Not sure why yet. |
7dde595 to
4be993b
Compare
|
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. |
| # in the STIX fonts, so we have to detect this one separately. | ||
| if (constants is STIXFontConstants and | ||
| isinstance(state.font_output, StixSansFonts)): | ||
| return STIXSansFontConstants |
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.
|
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. |
|
Pretty sure that is one of the pep8 errors/warnings we have blacklisted. |
|
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
|
|
👍 It looks like some of the symlog images are changed twice. Would it make sense to squash the commits? |
|
Sure, I can squash the commits. Also, let's hold off -- I think I just discovered another case where this isn't working... |
Fix failures in matplotlib#5416
b49f46b to
baa2b6e
Compare
|
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. |
| return make_cleanup | ||
| else: | ||
| result = make_cleanup(style) | ||
| style = 'classic' |
There was a problem hiding this comment.
ok, nevermind... I got mixed up
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.
There was a problem hiding this comment.
Perhaps make a comment here explaining that style in some cases isn't really a style name?
|
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. |
Travis looks green now. |
| # cleanup if called with an argument, it is a string naming a | ||
| # style, and the function will be passed as an argument to what we | ||
| # return. This is a confusing, but somewhat standard, pattern for | ||
| # writing a decorator with optional arguments. |
There was a problem hiding this comment.
Perfect! Very clear and helpful.
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? |
|
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. |
Make "classic" style have effect
|
Backported to 2.0.x as 10e2af5 |
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.fontsetrcParam. 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 whenmathtext.fontsetis "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.