API: Polar: allow flipped y/rlims....

[jklymak]

PR Summary

Closes #11202
Closes #10704

Update

Allow non-increasing ylimits:

• needs doc updates

import numpy as np
import matplotlib.pyplot as plt
fig = plt.figure()

ax = fig.add_axes([0.1, 0.1, 0.8, 0.8], polar=True)
ax.plot(np.arange(30), np.arange(30) / 30. + 1)
ax.set_ylim(2, 1)  # out of order, and hence didn't draw properly.

plt.show()

ax.set_rorigin(3.)

OLD:

This adds a check for set_ylim in polar to make sure they are in order and swap and emit an error if they are not. Because I had to overload axes.set_ylim I changed the argument handling a bit (hopefully correctly).

Fixes a typo in axes/_base.py

import numpy as np
import matplotlib.pyplot as plt

fig = plt.figure(dpi=120)

ax = fig.add_axes([0.1, 0.1, 0.8, 0.8], polar=True)

ax.set_theta_zero_location('N')
ax.set_theta_direction(-1)  # anti-clockwise

ax.set_ylim(90, -45)  # out of order, and hence didn't draw properly.  
ax.set_yticks(np.arange(-45, 90+0.1, 15))

plt.show()

PR Checklist

• Has Pytest style unit tests
• Code is Flake 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

[WeatherGod]

Devil's advocate here: (just about) everywhere else we are able to flip the order of the limits (particular favorite is pressure-as-height). Why shouldn't we be allowed to do something similar for polar plots?

[jklymak]

@WeatherGod Fine with me! But someone has to rewrite it so it works, which it does not right now...

[timhoffm]

If we plan to support an inverted axis at some point in the future, we should break with an error in that case for now.

While we can always move from an error to new functionality, going from auto-switching to inverted axis support would be a breaking change.

More philosophically, I feel that auto-switching may be too clever anyway. We can expect users to give the range in the correct order. If they don't, they either do it intentionally, because they expect a certain effect, or it's actually mistake, which might have other effects and go unnoticed because we corrected it for the plot ("but the plot looked correct").

So, I'm -1 on auto-switching.

[jklymak]

That makes sense. I’m fine w an error and if someone wants to implement inverted axes they can do so in future

[ImportanceOfBeingErnest]

I agree with @timhoffm here. You don't need to implement axis inversion here, but if you allow arbitrary order now, it'll be hard to allow for axis inversion via set_*lim in the future.

The error might point towards a possible workaround for now. For the r axis: "To mimic an inverted r-axis plot data.max()-data and set the ticklabels accordingly.". For theta axis: "Use set_theta_direction instead."

[jklymak]

Changed to a ValueError and changed the doc string to reflect thats what will happen.

I don't know that we need to point out all the possible work arounds in an error message - they tend to be hard to describe in a line or two. But if you had something pithy, that'd be fine.

[timhoffm]

I'm fine now with the functionality. Just some style issues.

[timhoffm]

See ~.polar.PolarAxes.set_ylim. Additionally, you can pass rmin/rmax in place of ymin/ymax.

[jklymak]

?? Not following this. We are trying to deprecate rmin, ymin etc. (Well, I don't agree with calling them bottom and top, but...)

[timhoffm]

Thought to document the current state, even if it's deprecated (well, which should be mentioned then). But since this had no documentation at all so far. It's also ok to keep this a secret.

Agreed on raising an error.

[timhoffm]

We don't have a consistent policy for None, True, False so far.

There are all styles present in our docs:

• plain
• literal: double backticks
• italics: starred

Personally, I find double backslashes too cumbersome to type and visually too broad in plain text for there short words. I just use *None*.

The numpydoc and sphinx examples just use plain text.

Just mentioning. Since we currently have no standards, I won't hold this up.

[jklymak]

Happy to adopt whatever standard folks like the best, or to start a standard! It was plain before, happy to put it back.

[jklymak]

... and I note I missed about half of them. Switched back to plain text.

[jklymak]

Added a small test for ValueError....

[jklymak]

squashed and rebased

[tacaswell]

It looks like this used to work with inverted axes, but we broke it (likely inadvertently) in the polar refactor. I think we should look into fixing it before we make it raise.

[tacaswell]

We should at least investigate why this broke.

Auto-inverting the axis direction for rectilinear plots (for better or worse), I don't see why radial plots should be different.

[jklymak]

Fair enough. I didn’t know it used to work. I’ll clowe and we can reopen if we decide we can’t fix it.

[WeatherGod]

Thomas, I think the issue with polar plots came about partly because we couldn't agree what to do about negative values, or radial limits that didn't start with 0. And allowing the bottom of a rectilinear plot to be 1000mb, and the top of the plot to be 10mb lets us all breathe!

…

On Sat, Sep 29, 2018 at 5:17 PM Thomas A Caswell ***@***.***> wrote: ***@***.**** requested changes on this pull request. We should at least investigate why this broke. Auto-inverting the axis direction for rectilinear plots (for better or worse), I don't see why radial plots should be different. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12300 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-EQj2_BB8_NE4b8wAp2HR7GdDYbwks5uf-N0gaJpZM4W7Wvv> .

[jklymak]

I don’t see any reason inverting the axis is difficult mathematically. I just assumed it wasn’t working on purpose.

I was confused by SO answers, I am not sure this ever worked anymore...

[tacaswell]

I just tested all they way back to 1.5 and this never actually worked (but silently kept going).

Sorry for the noise 🐑 .

[jklymak]

No problem - but OTOH, I don't actually see why this can't work. Marking WIP while I take a quick look...

[jklymak]

OK, this seems to work fine...

import numpy as np
import matplotlib.pyplot as plt
fig = plt.figure()

ax = fig.add_axes([0.1, 0.1, 0.8, 0.8], polar=True)
ax.plot(np.arange(30), np.arange(30) / 30. + 1)
ax.set_ylim(2, 1)  # out of order, and hence didn't draw properly.

plt.show()

ax.set_rorigin(3.)

Clearing because the PR is now not just raising an error

[jklymak]

Rebased....

[QuLogic]

Only some minor comments for now.

[QuLogic]

Something funny happens if you flip the radial limits after a draw:

#!/usr/bin/env python

import numpy as np
import matplotlib.pyplot as plt

fig, (ax0, ax1, ax2) = plt.subplots(1, 3, subplot_kw=dict(polar=True))

ax0.plot([0, 1], [-1, 1])
ax0.scatter([0, 1], [-1, 1])

ax0.set_rmin(-1.5)
ax0.set_rmax(1.5)
ax0.set_title('Forward')

ax1.plot([0, 1], [-1, 1])
ax1.scatter([0, 1], [-1, 1])

ax1.set_rmin(1.5)
ax1.set_rmax(-1.5)
ax1.set_title('Backward')

ax2.plot([0, 1], [-1, 1])
ax2.scatter([0, 1], [-1, 1])

ax2.set_rmin(-1.5)
ax2.set_rmax(1.5)
ax2.set_title('Backward with draw')
fig.canvas.draw()
ax2.set_rmin(1.5)
ax2.set_rmax(-1.5)

plt.show()

[jklymak]

@QuLogic Agreed, but that predates this PR, and is a different issue. On master:

import numpy as np
import matplotlib.pyplot as plt

fig, (ax0,  ax2) = plt.subplots(1, 2, subplot_kw=dict(polar=True))

ax0.plot([0, 1], [-1, 1])
ax0.scatter([0, 1], [-1, 1])

ax0.set_rmin(-1.5)
ax0.set_rmax(1.5)
ax0.set_title('Forward')


ax2.plot([0, 1], [-1, 1])
ax2.scatter([0, 1], [-1, 1])

ax2.set_rmin(-1.5)
ax2.set_rmax(1.5)
ax2.set_title('draw; change rlims')
fig.canvas.draw()
ax2.set_rmin(0)
ax2.set_rmax(1.5)

plt.show()

[QuLogic]

Some typos to fix, but otherwise good to go.

[jklymak]

Typos fixed - this has two approvals and could be merged if anyone has a minute...