Skip to content
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

[Bug]: unexpected thetalim behavior in polar plot #25568

Closed
ycopin opened this issue Mar 28, 2023 · 11 comments · Fixed by #27578
Closed

[Bug]: unexpected thetalim behavior in polar plot #25568

ycopin opened this issue Mar 28, 2023 · 11 comments · Fixed by #27578
Milestone

Comments

@ycopin
Copy link

ycopin commented Mar 28, 2023

Bug summary

I'd like to have a polar plot where angles go from -180° to +180°. It was my understanding that this was controled by set_thetalim, but this does not produce the expected result.

Code for reproduction

import numpy as np
import matplotlib.pyplot as plt

fig = plt.figure()

ax = fig.add_subplot(projection='polar')
ax.set_thetalim(-np.pi, +np.pi)   # [-180°, +180°]

plt.show()

Actual outcome

polar

Expected outcome

A plot where angles go from -180° to +180°.

Additional information

No response

Operating system

Ubuntu 20.04.6 LTS

Matplotlib Version

3.6.2

Matplotlib Backend

Qt5Agg

Python version

Python 3.8.10

Jupyter version

No response

Installation

pip

@ksunden
Copy link
Member

ksunden commented Mar 28, 2023

I think the actual problem has to do with the ThetaLocator getting confused by the overlapping limits.

When I do:

import numpy as np
import matplotlib.pyplot as plt

fig = plt.figure()

ax = fig.add_subplot(projection='polar')
ax.set_thetalim(-np.pi*.999, +np.pi)   # [-180°, +180°]

plt.show()

That is, just barely nudging so that the limits do not end up overlapping. I get:

Figure_1

Which, aside from the radial axis being shifted/having a line, is pretty much as expected.

It's a workaround rather than a proper fix, though.

Explicitly setting the ticks (with set_xticks, as set_thetaticks apparently doesn't exist...) is an actual fairily proper fix, though perhaps the default could be improved:

fig = plt.figure()
ax = fig.add_subplot(projection='polar')

ax.set_xticks(np.linspace(-np.pi, np.pi, 9)[1:])
ax.set_thetalim(-np.pi, +np.pi)   # [-180°, +180°]

plt.show()

Yields:
Figure_2

@ksunden
Copy link
Member

ksunden commented Mar 28, 2023

The following diff gets at least very close to expected behavior (I might prefer 180 as a label instead of -180, but they are equivalent so, not soo bad).

diff --git a/lib/matplotlib/projections/polar.py b/lib/matplotlib/projections/polar.py
index 9ac9bbe73c..3e4b1c8175 100644
--- a/lib/matplotlib/projections/polar.py
+++ b/lib/matplotlib/projections/polar.py
@@ -295,8 +295,9 @@ class ThetaLocator(mticker.Locator):
 
     def __call__(self):
         lim = self.axis.get_view_interval()
         if _is_full_circle_deg(lim[0], lim[1]):
-            return np.arange(8) * 2 * np.pi / 8
+            return (np.arange(8) * 2 * np.pi / 8) + np.deg2rad(min(lim))
         else:
             return np.deg2rad(self.base())

Yields (with the original code):

Figure_1

It also does pass all extant polar tests.

@ksunden
Copy link
Member

ksunden commented Mar 28, 2023

To summarize what was happening:

  • There is special handling in a few places for when the limits are a full circle.
    • This was correctly detecting that [-pi, pi] is a full circle
    • It is an error to be greater than one full circle
    • The case where it is not a full circle is what got the *.999 case to work mostly as expected
  • The default tick locations get only that it is a full circle
    • it defaults to 0, pi/4, pi/2, ..., 7*pi/4 rad (displayed in deg) (with no concept of the actual bounds)
    • half of those points are outside of the limits set by set_thetalim, so they are not shown
  • setting explicitly using set_xticks overrides that default
  • the proposed diff takes into account the minval (and since it is guaranteed to be within 1e-12 of a full circle, the whole thing)
    • including maxval instead of minval is a bit more complicated, and unclear what is desirable, as clearly in the 0-360 case, you want minval included. Not sure what the general solution would be.

@ycopin
Copy link
Author

ycopin commented Mar 29, 2023

The following diff gets at least very close to expected behavior (I might prefer 180 as a label instead of -180, but they are equivalent so, not soo bad).

In the best of worlds, I would suggest ±180 !

@timhoffm
Copy link
Member

timhoffm commented Mar 29, 2023

The following diff gets at least very close to expected behavior (I might prefer 180 as a label instead of -180, but they are equivalent so, not soo bad).

This is already a 90% solution. IMHO good enough for a fix if we don't want to spend more time.

If you want to go further, you'd need to distinguish between minval and maxval tick limits. Up to discussion, but I'd argue that you typically want the maxval, e.g. (-90°, 270°], (-180°, 180°]. Only [0*, 360°) wants the minval.

Untested: It may be enough to shift the tick positions:

if min(lim) == 0:
    return (np.arange(8) * 2 * np.pi / 8) + np.deg2rad(min(lim))
else:
    return (np.arange(1, 9) * 2 * np.pi / 8) + np.deg2rad(min(lim))

In the best of worlds, I would suggest ±180 !

This would additionally need to be handled in ThetaFormatter.__call__
https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/projections/polar.py#LL245-L253

One can obtain self.locs, to format depending on the available tick locations.

@ksunden
Copy link
Member

ksunden commented Mar 29, 2023

It's not obvious to me that 270 is the preferred label for the [-90, 270] case. In fact, I would tend to lean towards "smallest absolute value gets the label, with +180 preferred over -180" if I were to flesh it out further beyond the simple "offset by minval".

Another example that is not too unreasonable:

[720,1080] I would expect the minval there

@Higgs32584
Copy link
Contributor

@ksunden @timhoffm

Since there appears to be such a strong preference on either side maybe we should just set it blank and have the user set it.

Also, on sundens point of polar angle validation, I do not know if it exists or not, but there should be a way to turn it off. I can still see someone wanting to map non integer values

@timhoffm
Copy link
Member

I don't have a strong preference. "Smallest absolute value" seems reasonable too.

@ksunden
Copy link
Member

ksunden commented Mar 29, 2023

I would also not describe my preference as "strong", quite the opposite in fact. My position has been "unclear what is desirable" in many cases. I came up with a heuristic that I think encapsulates my expectations, but still considering options.

Leaving blank would be unexpected to me, as that would break the symmetry of where gridlines/ticks are placed. This is about defaults, as of course you can set them to whatever you like as the end user.

@Higgs32584 I am not sure what you mean by "polar angle validation" here. Nothing in this discussion has been limited to integer values, as far as I can tell.

The proposed diff (and variations on that theme, modulo which end gets kept) will work perfectly fine with e.g ax.set_thetalim(thetamin=22.5, thetamax=382.5) (using the keywords to set in degrees so I don't have to convert to rad and back). At which point the ticks will be at [22.5, 67.5, 112.5, ... 337.5]. Not quite sure what the default ThetaFormatter will do, it may display as integers (simply by virtue of rounding such that the labels remain a reasonable length).

@Higgs32584
Copy link
Contributor

When I meant polar Angle validation I meant this part:

"Another example that is not too unreasonable:

[720,1080] I would expect the minval there"

I apologize for calling your comments above "strong"

@ksunden
Copy link
Member

ksunden commented Mar 30, 2023

I still do no understand what should be "turned off" about that.

This particular code path only affects full circles, there is separate handling for non-full circle, which uses an underlying locator implementation.

You can call ax.xaxis.set_major_locator(...) to get other behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants