Bugfix for issue 16501 raised ValueError polar subplot with (thetamax - thetamin) > 2pi #16717
Conversation
| if 'thetamin' in kwargs: | ||
| kwargs['xmin'] = np.deg2rad(kwargs.pop('thetamin')) | ||
| thetamin = np.deg2rad(kwargs.pop('thetamin')) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, the if statements there are to check the possible signatures. The signatures allowed for set_thetalim are as follows:
- set_thetalim(minval, maxval): Set the limits in radians.
- 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 'thetamin' in kwargs: | ||
| kwargs['xmin'] = np.deg2rad(kwargs.pop('thetamin')) | ||
| thetamin = np.deg2rad(kwargs.pop('thetamin')) |
There was a problem hiding this comment.
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
|
@dstansby np glad we could help!! |
PR Summary
Recopy of old PR: #16711.
This PR addresses issue #16501 set_thetalim takes in parameters as radians or degrees but the problem occurs when the range or difference between the parameters is large than 2pi radians or larger than 360 in terms of degrees. The polar subplot will produce a deformed graph as it is shown in the Github issue 16501.
Fix: The fix was done using a check for ranges larger than 2pi, and if so ValueError is raised
PR Checklist