Skip to content

Polar tick improvements #9068

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

Merged
merged 15 commits into from
Sep 26, 2017
Merged

Polar tick improvements #9068

merged 15 commits into from
Sep 26, 2017

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Aug 21, 2017

PR Summary

Semi-WIP as some bits of this are a bit hacky, so they might need to be corrected a bit. This improves upon ticks used in polar plots by correctly rotating them to be perpendicular to the spine. Tick labels also now use the configured padding instead of some fixed value radially; this is a big difference if the radial limits are small or large.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@QuLogic
Copy link
Member Author

QuLogic commented Aug 28, 2017

Would like to get this in to 2.1, since it's a good follow-on to #4699. Just need to figure out why it's not passing somehow.

@QuLogic QuLogic added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 28, 2017
@QuLogic QuLogic force-pushed the polar-ticks branch 2 times, most recently from df70809 to c753077 Compare August 29, 2017 07:33
@QuLogic
Copy link
Member Author

QuLogic commented Aug 29, 2017

Ah, all set now; I wonder if that reset_ticks change should be moved into the main function?

@tacaswell
Copy link
Member

The r-grid ticks do not look like they are pulling the tangent from the right place?

Over all 👍

@dstansby
Copy link
Member

dstansby commented Aug 29, 2017

I think the new label directions are much more un-readable than having them all horizontal. It also doesn't look like there are any image tests for angular ticks on the outside of the axes?

@dopplershift
Copy link
Contributor

I'm +0.5 on having the capability to turn on the rotated labels, -1000 on having it be the default. This belonged in 2.0 if we were going to do it since this is a stylistic change--I really don't think our users will view this as a clear enhancement. (My first reaction honestly was "Ewww.")

@WeatherGod
Copy link
Member

WeatherGod commented Aug 29, 2017 via email

@efiring
Copy link
Member

efiring commented Aug 29, 2017

This could be a useful option--but only if the upside-down labels are flipped 180 degrees to bring them right-side-up. In other words, label orientations should be kept within +-90 degrees, not +-180.

@WeatherGod
Copy link
Member

WeatherGod commented Aug 29, 2017 via email

@dopplershift
Copy link
Contributor

So an informal survey by posting an image (from the test suite changes) to Twitter would seem to argue that this change is by no means a slam dunk in terms of improving default plot quality. I do think there's a strong case to be made for having the option to turn this behavior on, though.

@afvincent
Copy link
Contributor

FWIW, even if one decides to keep the current default behavior, I would advocate for keeping the 2 new polar-tick classes and an ad-hoc example somewhere.

IMHO, the rotated radial ticks could sometimes be more readable than the current ones, especially when cluttering is not far away, for example (from the test images):
polar_proj_current
vs.
polar_proj_rotated
Even though the anchor for rotation seems a bit weird to me, it is rather clear to me which label corresponds to which circle. With the current behavior (and apart from the problem of the radial tick labels overlapping each other...), I challenge anybody to say the same for the intermediate values without counting the circles and the tick labels from one edge or the other 😈.

For the theta-tick label, I think that having an option to limit the rotation domain as @efiring suggested, as well as @WeatherGod´s one about allowing the labels to be parallel to the normal could be nice additions. If I may, I would also suggest to have an option to rotate the theta-tick labels outwards, i.e. + 180° compared to the current rotated ones. Possible uses of rotated theta-tick labels that come to my mind are sky/constellation charts (random examples here and here)

@QuLogic
Copy link
Member Author

QuLogic commented Aug 30, 2017

There is certainly a tradeoff involved between full-circle plots and wedges (and I'm probably favouring the latter a bit). Consider
figure_1
vs
figure_2
The radial ticks could be rotated manually (but it'd be a pain if you modified the limits) but angular ticks would be much more annoying. That being said, there are already several special cases for full-circle plots and having it remain (almost) the same as before is certainly feasible.

The r-grid ticks do not look like they are pulling the tangent from the right place?

Even though the anchor for rotation seems a bit weird to me,

They are anchored to fit with ticks. However, since ticks have always been broken in polar plots, they're off by default which makes the labels seem oddly positioned. Here's a plot with the ticks on:
figure_3
I'm not totally sure how well parallel ticks work on a full-circle plot. It might be nicer to have the labels "above" the circle.

This could be a useful option--but only if the upside-down labels are flipped 180 degrees to bring them right-side-up. In other words, label orientations should be kept within +-90 degrees, not +-180.

Easy to do, really, though I guess the question is about customization then and how easy that is to override. Especially tied into the next point.

Just took a look at the rotatelabels feature of pie(), and it made me realize that there might be two ways people would want the labels oriented: perpendicular to the normal, or parallel to the normal.

We already have support for this in Cartesian plots: ax.xaxis.set_tick_params(rotation=90) and it mostly works here too except the anchor point is wrong (I can investigate that later.)

@QuLogic
Copy link
Member Author

QuLogic commented Aug 30, 2017

To summarize, I'd like this on for wedges, but there already many special cases to preserve the old behaviour with full circle plots, so I'm fine with adding a few more to do the same for tick labels. I don't think there's any point in preserving old ticks themselves because they were useless before.

@QuLogic QuLogic added this to the 2.1 (next point release) milestone Aug 30, 2017
@dopplershift
Copy link
Contributor

To be clear I think the new code with proper polar ticks is a nice upgrade. I just think the new alignment/rotation constitutes a style change that would not be universally well-received, so maintaining the old look (at least for full-circle plots) is the way to go.

@dopplershift
Copy link
Contributor

Also, I'm 👎 on holding up cutting the RC for this, so somebody had better get working on the modifications to make this behavior optional if it's going to land in the RC this week. 😈

@anntzer
Copy link
Contributor

anntzer commented Aug 30, 2017

2c from the peanut gallery: the rotated labels would probably look better if their "horizontal" (= in the axis of the text) center was tangent to the circle, not their "left" edge.

@QuLogic
Copy link
Member Author

QuLogic commented Aug 30, 2017

It's straightforward to replicate old behaviour because there are already several checks for full-circle vs wedges. The only problem is that now that we are properly interpreting tick padding, the default is too small on figures of the size in the tests. Note though that since previously the angles were just offset by a fixed radial distance, any other sized figure might have had too much or too little padding with no way to fix it. I'm not really sure what to do about this. Maybe just add extra padding automatically?

@QuLogic
Copy link
Member Author

QuLogic commented Aug 31, 2017

I'm not totally sure I like that hack to get padding to match previous tests. Maybe the best option is to add an rcParam for polar plots with a better default (but this is a lot more work)?

@tacaswell
Copy link
Member

On a bit more consideration, I am convinced that changing the default rotation is a style change which we have already determined is an API break so 👍 to these classes going in 👎 to rotating the tick labels by default on full circles 👍 to rotated by default on wedges (as those are new) and 👍 to a convenience method on the axes objects to apply the rotation.

I am 50/50 on letting this be merged post RC assuming it can be made to not break tests.

@QuLogic
Copy link
Member Author

QuLogic commented Aug 31, 2017

Current results are 99% the same; it's very hard to get the padding exactly the same since it was never quite correct in the first place.

I keep forgetting that there are a couple polar tests that output multiple files, so that's why tests are failing right now.

@tacaswell
Copy link
Member

I am fine with minor padding shifts.

@tacaswell
Copy link
Member

https://github.com/matplotlib/matplotlib/pull/9068/files#diff-f1d864385683ce9e37a13df3975627c9 the labels have been squished up into the ticks.

@QuLogic
Copy link
Member Author

QuLogic commented Sep 25, 2017

Sure, give me a couple hours to make sure everything works and I can take care of it.

This option enables the automatic rotation of polar tick labels.
@QuLogic
Copy link
Member Author

QuLogic commented Sep 25, 2017

I had rebased this earlier today and added the 'auto' option. Just now I made a small tweak so that 'auto' will also apply to full circles (since presumably that's what the user wants if they've explicitly asked for it) and modified one test to check that that works.

The last two commits (before the image change) are the new things.

@@ -62,6 +62,13 @@ negative values are simply used as labels, and the real radius is shifted by
the configured minimum. This release also allows negative radii to be used for
grids and ticks, which were previously silently ignored.

Radial ticks have be modified to be parallel to the circular grid line. Angular
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, small grammar typo here.

@tacaswell tacaswell merged commit 4792425 into matplotlib:master Sep 26, 2017
tacaswell added a commit that referenced this pull request Sep 26, 2017
ENH: Polar tick improvements
@tacaswell
Copy link
Member

backported to v2.1.x as 6889c6d

@QuLogic QuLogic deleted the polar-ticks branch September 26, 2017 03:18
@pmcculey
Copy link

pmcculey commented Dec 8, 2017

if rotation of the theta labels is used in conjunction with ax.set_theta_zero_location( "N") and or ax.set_theta_direction( 1 ), the labels seem to rotate weirdly as you go around the theta axis. The following code should illustrate what I mean, the label flips through 180 degrees as you move from 90 to 135. I'm on a windows 10 PC & using python 2.7 btw.

import matplotlib.pyplot as plt
import numpy as np

fig = plt.figure()
rect = 0,0,1,1
ax = fig.add_axes(rect, label = 'ax', polar=True)
ax.patch.set_alpha(0)
ax.set_theta_zero_location( "N" )
ax.set_theta_direction( 1 )
thetaticks = np.arange( 0, 360, 45 )
ax.tick_params( 'x', pad = -15, rotation = 'auto' )

plt.show()

@QuLogic
Copy link
Member Author

QuLogic commented Dec 9, 2017

That is fixed by #9881.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: polar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants