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
Appearance settings

Bugfix for issue 16501 raised ValueError polar subplot with (thetamax - thetamin) > 2pi #16717

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.

Sign up for GitHub

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 7 commits into from
Apr 19, 2020
+44 −4
Merged

Bugfix for issue 16501 raised ValueError polar subplot with (thetamax - thetamin) > 2pi #16717

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a6929f3
fixed theta > 2pi bug issue # 16501
prasadve Mar 7, 2020
942ee4d
added unit tests for issue #16501
prasadve Mar 7, 2020
fe18d37
made code flake 8 compatible
prasadve Mar 8, 2020
315a522
added some document for issue #16501
prasadve Mar 8, 2020
377abe0
added explicit call and more tests #16501
prasadve Apr 1, 2020
a69db44
changed order, remove braces on if and changed error message
prasadve Apr 2, 2020
e38a5bc
flake8 #16501
prasadve Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions 26 lib/matplotlib/projections/polar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -987,13 +987,31 @@ def set_thetalim(self, *args, **kwargs):
where minval and maxval are the minimum and maximum limits. Values are
wrapped in to the range :math:`[0, 2\pi]` (in radians), so for example
it is possible to do ``set_thetalim(-np.pi / 2, np.pi / 2)`` to have
an axes symmetric around 0.
an axes symmetric around 0. A ValueError is raised if the absolute
angle difference is larger than :math:2\pi.
dstansby marked this conversation as resolved.
Show resolved Hide resolved
"""
thetamin = None
thetamax = None
left = None
right = None

if 'thetamin' in kwargs:
kwargs['xmin'] = np.deg2rad(kwargs.pop('thetamin'))
thetamin = np.deg2rad(kwargs.pop('thetamin'))
Copy link
Member

Choose a reason for hiding this comment

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

Is all this argument dance because we have to check the generic *args, *kwargs signature? If so, we should parse to that thetamax and thetamin, check that once and pass it to set_xlim() explicitly.

Later on, we should make the method signature explicit, which will simplify the check. Ideally, one would to this before adding the check, but since that will involve an API-change with a deprecation period, we would not be able to add the simplified check for the next to minor releases. So the more complex check is necessary for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the if statements there are to check the possible signatures. The signatures allowed for set_thetalim are as follows:

  1. set_thetalim(minval, maxval): Set the limits in radians.
  2. set_thetalim(thetamin=minval, thetamax=maxval): Set the limits in degrees.
    These were taken straight out of the documentation for function set_thetalim.

By "If, so we should parse to that thetamax and thetamin, check that once and pass it to set_xlim() explicitly" do you mean signature 1 ( radians ) would not be used anymore?

Copy link
Member

Choose a reason for hiding this comment

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

Ultimately (unless we decide to change the awkward rad/degree API altogether) we could at least do def set_thetalim(min_rad, max_rad, /, *, thetamin, thetamax), which is more explicit but has the exact same semantics in terms of positional and keyword arguements. But that would have to wait until the min. supported Python version is 3.8, which introduced the positional-only arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for this change I have made changes. Made an explicit call to set_xlim using parameter thetamin, thetamax, left and right. Please let me know if further changes need to be made. I do agree making the set_thetalim explicit in the future when 3.8 rolls out.

Copy link
Member

Choose a reason for hiding this comment

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

Passing the parameters on consistently and checking them on the go is surprisingly complex. This code covers the documented API versions using two kwargs or two positional args. It does not catch all edge cases like set_thetalim(0, thetamax=180 which technically works as well.

I'll think about this but I think that's ok.

I'd move these theta parsing down so that you have the order

  • args parsing
  • args checks
  • theta parsing
  • theta checks

if 'thetamax' in kwargs:
kwargs['xmax'] = np.deg2rad(kwargs.pop('thetamax'))
return tuple(np.rad2deg(self.set_xlim(*args, **kwargs)))
thetamax = np.deg2rad(kwargs.pop('thetamax'))

if len(args) == 2:
if args[0] is not None and args[1] is not None:
left = args[0]
right = args[1]
dstansby marked this conversation as resolved.
Show resolved Hide resolved
if(abs(right - left) > 2 * np.pi):
dstansby marked this conversation as resolved.
Show resolved Hide resolved
raise ValueError('Cannot pass angle range > 2 pi')
dstansby marked this conversation as resolved.
Show resolved Hide resolved

if(thetamin is not None and thetamax is not None):
dstansby marked this conversation as resolved.
Show resolved Hide resolved
if(abs(thetamax - thetamin) > 2 * np.pi):
dstansby marked this conversation as resolved.
Show resolved Hide resolved
raise ValueError('Cannot pass angle range > 2 pi')
dstansby marked this conversation as resolved.
Show resolved Hide resolved
return tuple(np.rad2deg(self.set_xlim(left=left, right=right,
xmin=thetamin, xmax=thetamax)))

def set_theta_offset(self, offset):
"""
Expand Down
22 changes: 22 additions & 0 deletions 22 lib/matplotlib/tests/test_subplots.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,25 @@ def test_dont_mutate_kwargs():
gridspec_kw=gridspec_kw)
assert subplot_kw == {'sharex': 'all'}
assert gridspec_kw == {'width_ratios': [1, 2]}


def test_subplot_theta_min_max_raise():
with pytest.raises(ValueError, match="Cannot pass angle range > 2 pi"):
ax = plt.subplot(111, projection='polar')
ax.set_thetalim(thetamin=800, thetamax=400)
dstansby marked this conversation as resolved.
Show resolved Hide resolved


def test_subplot_theta_min_max_non_raise():
ax = plt.subplot(111, projection='polar')
ax.set_thetalim(thetamin=800, thetamax=440)


def test_subplot_theta_range_raise():
with pytest.raises(ValueError, match="Cannot pass angle range > 2 pi"):
ax = plt.subplot(111, projection='polar')
ax.set_thetalim(0, 3 * numpy.pi)


def test_subplot_theta_range_normal_non_raise():
ax = plt.subplot(111, projection='polar')
ax.set_thetalim(0, 2 * numpy.pi)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.