Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
mdboom wants to merge 6 commits into matplotlib:master from mdboom:3d-tick-width

Conversation

Copy link
Member

@mdboom mdboom commented Nov 30, 2015

More-or-less by accident, the line width for ticks in mplot3d came from lines.linewidth, rather than xtick.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 and ytick here -- the tick widths don't seem to have settings for each individual axis in mplot3d.

Cc: @WeatherGod as the 3d guru

@mdboom mdboom added this to the next major release (2.0) milestone Nov 30, 2015
Copy link
Member

👍 It looks like one of the tests has changed enough fail. I guess we should just update the base line image.

Copy link
Member

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.

Copy link
Member Author

mdboom commented Nov 30, 2015

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.

@mdboom mdboom changed the title (削除) Get tickmark linewidth from style in 3d (削除ここまで) (追記) Use various rcParam properties in 3d (追記ここまで) Nov 30, 2015
Copy link
Member

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...

Copy link
Member

We should probably add a whatsnew that states that mplot3d now respects even more rcParams.

Copy link
Member

Hmmm, I am guessing that Cyber Monday is putting some strain on AWS? Guess I will see how this turns out tomorrow.

Copy link
Member Author

mdboom commented Dec 1, 2015

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.

Copy link
Member

I wonder why the "Z LABEL" moved so much in the labelpad example?

Copy link
Member Author

mdboom commented Dec 1, 2015

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.

Copy link
Member Author

mdboom commented Dec 1, 2015

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Member Author

mdboom commented Dec 1, 2015

Thanks for the reminder about #5447. Not exactly sure what's up there.

@mdboom mdboom force-pushed the 3d-tick-width branch 2 times, most recently from 8f3c3f7 to 8913503 Compare December 14, 2015 15:32
Copy link
Member

@mdboom can you re-generate the images?

@WeatherGod You are in charge of merging this one.

Copy link
Member

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.

Copy link
Member

efiring commented Feb 10, 2016

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.

Copy link
Member

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.

Copy link
Member

QuLogic commented May 16, 2016

Two weeks to go, so ping. This needs a rebase, too.

Copy link
Member

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).

Copy link
Member

Ping on the rebase. I think this should go into v2.0.

Copy link
Member

QuLogic commented Jul 7, 2016

Ping again?

Copy link
Member

@mdboom, do you need me to take over this one?

Copy link
Member

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)

@@ -13,7 +13,7 @@
# set this to True. It will download and build a specific version of
# FreeType, and then use that to build the ft2font extension. This
# ensures that test images are exactly reproducible.
#local_freetype = False
local_freetype = False
Copy link
Member

@WeatherGod WeatherGod Jul 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change unintentional?

'color': rcParams['axes.edgecolor']},
'grid' : {'color': rcParams['grid.color'],
'linewidth': rcParams['grid.linewidth'],
'linestyle': rcParams['grid.linestyle']},
Copy link
Member

@WeatherGod WeatherGod Jul 16, 2016

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.

Copy link
Member

Superseded by #6765

@QuLogic QuLogic modified the milestones: unassigned, 2.0 (style change major release) Jul 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /