8000 Bugfix for issue 16501 raised ValueError polar subplot with (thetamax - thetamin) > 2pi by prasadve · Pull Request #16717 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

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
Merged

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

Show file tree
Hide file tree
Changes from all 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
25 changes: 21 additions & 4 deletions 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,30 @@ 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`.
"""
thetamin = None
thetamax = None
left = None
right = None

if len(args) == 2:
if args[0] is not None and args[1] is not None:
left, right = args
if abs(right - left) > 2 * np.pi:
raise ValueError('The angle range must be <= 2 pi')

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 thetamin is not None and thetamax is not None:
if abs(thetamax - thetamin) > 2 * np.pi:
raise ValueError('The angle range must be<= 360 degrees')
return tuple(np.rad2deg(self.set_xlim(left=left, right=right,
xmin=thetamin, xmax=thetamax)))

def set_theta_offset(self, offset):
"""
Expand Down
23 changes: 23 additions & 0 deletions 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,26 @@ 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='The angle range ' +
'must be<= 360 degrees'):
ax = plt.subplot(111, projection='polar')
ax.set_thetalim(thetamin=800, thetamax=400)


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='The angle range must be <= 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)
0