-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Use various rcParam properties in 3d #5585
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
Conversation
👍 It looks like one of the tests has changed enough fail. I guess we should just update the base line image.
I wouldn't do it this way. I have no clue why a function called "tick_update_position" would ever had had anything to do with setting visual properties, so lets not add to that. Instead, it might make sense to add this information to the kludgy dictionary update around L85. You will also have access to the adir
variable, which has values of "x", "y", and "z", which would help in obtaining the correct rcParam. Then, around L416, you can set the tick linewidth from the info['tick']
dictionary.
Good points. I actually discovered a number of other things that are hardcoded that should probably be borrowed from the appropriate rcParams. Renaming this PR to reflect increased scope.
Looks good to me. We will see how Travis likes it shortly.
Someday, I should look into why more of this wasn't offloaded to the base axes/axis class in the first place...
We should probably add a whatsnew that states that mplot3d now respects even more rcParams.
Hmmm, I am guessing that Cyber Monday is putting some strain on AWS? Guess I will see how this turns out tomorrow.
I've updated the tests. Unfortunately, some of these tests are also hit by the bug in #5531. We'll probably need to wait until that's fixed and then regenerate the test images here.
I wonder why the "Z LABEL" moved so much in the labelpad example?
I'm not sure, but that change has actually been there a while, just not caught by the test suite. I'll see if I can bisect it.
I rolled all the way back to when that labelpad
test was introduced, and it behaves like it does today -- with the Z LABEL in a different position than the baseline image. Chalk this one up to another failure in the fuzzy testing system. I think the current behavior is objectively more correct anyway -- I think it's fine to just update the baseline image like this in this case.
Just noticed that there is a grid
method that should probably get hooked up properly. Currently, it ignores all keyword arguments because all of the grid parameters were hardcoded anyway.
That is curious that the image was always wrong. Perhaps it was generated in an earlier version of the original PR. I should note that there was a regression issue filed about a month ago on the tickpadding for v1.5.0: #5447
Thanks for the reminder about #5447. Not exactly sure what's up there.
8f3c3f7
to
8913503
Compare
@mdboom can you re-generate the images?
@WeatherGod You are in charge of merging this one.
Was some of these commits merged as part of the 2.0 style change work? I don't know how else the tickwidths could get larger as reported in #5981.
On 2016年02月10日 8:00 AM, Benjamin Root wrote:
Was some of these commits merged as part of the 2.0 style change work? I
don't know how else the tickwidths could get larger as reported in #5981
#5981.
Yes, Tom and I merged some PRs in an attempt to get things moving
towards 2.0. This is not to preclude tweaks and changes, but rather to
speed up the review and to reveal what works and what doesn't.
Not a problem, we just need to figure out what is and isn't in v2.x and master, I think, in order to properly complete this PR.
Two weeks to go, so ping. This needs a rebase, too.
This looks pretty good, and should solve a lot of issues. It does need a rebase though, and possibly a regeneration of the test images (the 2D plot has inward facing ticks).
Ping on the rebase. I think this should go into v2.0.
Ping again?
@mdboom, do you need me to take over this one?
8913503
to
756cf3d
Compare
Travis completely fails across all jobs:
FAIL: matplotlib.tests.test_collections.test_polycollection_close.test
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
self.test(*self.arg)
File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 55, in failer
result = f(*args, **kwargs)
File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 266, in do_test
'(RMS %(rms).3f)'%err)
ImageComparisonFailure: images not close: /home/travis/build/matplotlib/matplotlib/result_images/test_collections/polycollection_close.png vs. /home/travis/build/matplotlib/matplotlib/result_images/test_collections/polycollection_close-expected.png (RMS 8.256)
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.
Was this change unintentional?
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.
We might need to do a "classic mode" shim here. In v1.5, the grid lines used the linestyle, linewidth, and color that were the defaults for lines, not the defaults for grids. We might need a bunch of ternary statements or just a single if-statement populating this update dictionary. Any new entries should fall back to None
so that the old behavior is preserved.
Superseded by #6765
More-or-less by accident, the line width for ticks in mplot3d came from
lines.linewidth
, rather thanxtick.major.width
. This was unnoticeable when the two values are the same by default, but with the 2.0 style changes they are now different.I don't know if we should take the average of
xtick
andytick
here -- the tick widths don't seem to have settings for each individual axis in mplot3d.Cc: @WeatherGod as the 3d guru