Check for non-finite axis limits placed on converted_limit #8474
Conversation
|
@QuLogic please take a look |
ab76bbc to
a6b53b0
Compare
|
Hold on a second here... why isn't this method in the base class? Is the check different in the Axes class? I just did a quick looking around at the other PRs related to this one, and they don't seem to do the check in the same way. I am confused. |
|
In the |
|
Yes, I was going to suggest the same thing; move implementation into the base class and use it there as well. |
|
@WeatherGod @QuLogic moved it to the Base class and replaced the checks there with calls to |
4e22479 to
c6e151f
Compare
|
I just noticed that the 3d limit functions now converts the limit twice. I don't think we have any tests that applies units for 3d axes, so this wouldn't have been noticed in the previous PR. |
lib/matplotlib/axes/_base.py
Outdated
| def _validate_axis_limits(self, limit, convert): | ||
| """ | ||
| Raise ValueError if specified axis limits are infinite. | ||
|
|
There was a problem hiding this comment.
The method name is a bit misleading. It validates the converted limit. Also, the docstring doesn't say that it returns anything useful, nor that None can be an input.
There was a problem hiding this comment.
I'll change the name to _validate_converted_limits . Should I change the docstring to """ Raise ValueError if converted limits are non-finite.
Note that this functions also accepts None as a limit argument.
Returns : limit value after call to convert_xunits() or convert_yunits() or convert_zunits()""" ?
There was a problem hiding this comment.
Name is good. A slight tweak to that docstring,
"Returns : The limit value after call to convert(), or None if limit is None."
lib/matplotlib/axes/_base.py
Outdated
| raise ValueError("Specified x limits must be finite; " | ||
| "instead, found: (%s, %s)" % (left, right)) | ||
| left = self._validate_axis_limits(left, self.convert_xunits) | ||
| right = self._validate_axis_limits(right, self.convert_xunits) |
There was a problem hiding this comment.
This is so much cleaner! Thank you for this!
|
I checked the 3d functions, |
|
Fix it here in this PR. It is relevant to what you are fixing up. |
|
@WeatherGod pushed the suggested changes. Please take a look. |
There was a problem hiding this comment.
Just a couple of typos.
One thing that we lose with this overall change that was nice is the exception message saying which axis was problematic; it also said what the original limits were. I think the trade-off is fine since we can figure out the problematic axis by looking at the traceback, and debugging can get us the values.
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
| bottom = self._validate_axis_limits(bottom, self.convert_yunits) | ||
| top = self._validate_axis_limits(top, self.convert_yunits) | ||
| bottom = self._validate_converted_limits(bottom, self.convert_yunits) | ||
| top = self._validate_converted_limits(top, self.convert_yunits) |
There was a problem hiding this comment.
convert_yunits --> convert_zunits
lib/matplotlib/axes/_base.py
Outdated
| """ | ||
| Raise ValueError if converted limits are non-finite. | ||
|
|
||
| Note that this functions also accepts None as a limit argument. |
|
Fixed. Thanks! |
There was a problem hiding this comment.
More lines removed than added: I love it! I'll wait for the CI and @WeatherGod to change his review status before merging.
|
Will merge once Travis and friends are done. Thanks for bearing with me, and sorry for not noticing these issues in the previous PRs. |
|
Not an issue and thank you too for helping me :) |
In addition to my previous PR ( #8022 ), this one places the check in the
_validate_axes_limits()function in/lib/mpl_toolkits/mplot3d/axes3d.pyon the converted limits rather than on the limits passed to the function