-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fixed Issue #7460: Raised an error if argument to xlim is invalid #8022
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.
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
Conversation
Can you please add a test ? |
lib/matplotlib/axes/_base.py
Outdated
@@ -2871,8 +2871,12 @@ def set_xlim(self, left=None, right=None, emit=True, auto=False, **kw): | |||
|
|||
self._process_unit_info(xdata=(left, right)) | |||
if left is not None: | |||
if isinstance(left,float) and (math.isnan(left) or np.isinf(left)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this going to work if passed a numpy float?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps numpy.isreal
and numpy.isfinite
would be better to use here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it still works, but I think we should leverage as much as possible numpy considering it is one of our dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think of it this way. We recently discovered some bugs that were because numpy ints and python ints aren't the same in py3k, and so isinstance(a, int)
of a numpy int wouldn't work anymore. Better safe than sorry.
what about |
Something has gone very wring with git here....What were you trying to do? Probably worth jumping in https://gitter.im/matplotlib/matplotlib to get help sorting this out. |
I updated my local fork's master to be in sync with the upstream : matplotlib/matplotlib (master) and then force pushed the master to my local fork's branch(which is linked to this PR). I guess that is what made things go very wrong. Pruning isn't working. What do you suggest i do? |
Don't push master to the branch -- you do rebases instead, or at least do
merges (but we prefer rebases). Never do pushes like that.
If you jump onto gitter, some very savy git users can use various reflog
tricks to hopefully fix your branch.
…On Wed, Mar 1, 2017 at 1:41 AM, Vedant Nanda ***@***.***> wrote:
I updated my local fork's master to be in sync with the upstream :
matplotlib/matplotlib (master) and then force pushed the master to my local
fork's branch(which is linked to this PR). I guess that is what made things
go very wrong. Pruning isn't working. What do you suggest i do?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#8022 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-JDaHXoGBzc-7yZxhcFIy8p24IhHks5rhRMegaJpZM4L3jpJ>
.
|
i have reverted the merge which led to all this. Now the branch is in the right state, however it shows 226 commits. Perhaps best to close this PR and open a new one? |
no... do an interactive rebase to get rid of them all. It is called rebase
and squash.
…On Wed, Mar 1, 2017 at 1:36 PM, Vedant Nanda ***@***.***> wrote:
i have reverted the merge which led to all this. Now the branch is in the
right state, however it shows 226 commits. Perhaps best to close this PR
and open a new one?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#8022 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-AwUusy7aj1Z1MGaJ-Xsfh3LGfRWks5rhbqfgaJpZM4L3jpJ>
.
|
Okay done, thanks a lot for the help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
travis errors indicate that the 3d tests are incorrect. See below.
with pytest.raises(ValueError) as err: | ||
plt.set_xlim3d(right=np.nan) | ||
with pytest.raises(ValueError) as err: | ||
plt.set_xlim3d(right=np.inf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These won't work because the default axes object do not have these methods. You will need to explicitly create the 3d axes object and call the methods on it.
with pytest.raises(ValueError) as err: | ||
plt.set_zlim3d(top=np.inf) | ||
|
||
err.match('NaN or Inf cannot be the argument values') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not an expert in pytest, but I am not quite sure this is correct. Wouldn't the err
object get clobbered in each test, so you are really only comparing with the last one?
lib/matplotlib/tests/test_axes.py
Outdated
with pytest.raises(ValueError) as err: | ||
plt.set_xlim(left=np.nan) | ||
plt.xlim(left=np.nan) | ||
assert err.value.message == error_string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do exceptions have this message
attribute in py3k? The test suite is failing for py3k, saying that it doesn't (but it works for python27).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
along those lines, I'm comfortable with simply checking the error type
@phobson @WeatherGod I've added tests that simply check error type. Travis and Appveyor both build successfully now. I hope this is fine. |
lib/matplotlib/axes/_base.py
Outdated
@@ -2874,8 +2874,14 @@ def set_xlim(self, left=None, right=None, emit=True, auto=False, **kw): | |||
|
|||
self._process_unit_info(xdata=(left, right)) | |||
if left is not None: | |||
if (isinstance(left, float) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is code block is repeated several times throughout the PR.
i recommend factoring it out into its own method. something like:
...
class _AxesBase(martist.Artist):
...
def _validate_axis_limit(limit):
if limit is not None:
if isinstance(limit, float) and
(not np.isreal(limit) or not np.isfinite(right)):
raise ValueError(...)
return self.convert_xunits(limit)
and then all of these blocks can just be, e.g.
right = self._validate_axis_limit(right)
lib/matplotlib/tests/test_axes.py
Outdated
@@ -4972,3 +4972,23 @@ def test_bar_single_height(): | |||
ax.bar(range(4), 1) | |||
# Check that a horizontal chart with one width works | |||
ax.bar(0, 1, bottom=range(4), width=1, orientation='horizontal') | |||
|
|||
|
|||
def test_invalid_axes_limits(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should try to parametrize the tests too:
a first pass would be something as simple as:
@pytest.mark.parametrize('value', [np.inf, np.nan])
def test_invalid_axes_limits(value)
with pytest.raises(ValueError):
plt.xlim(left=value)
with pytest.raises(ValueError):
plt.xlim(right=value)
with pytest.raises(ValueError):
plt.ylim(bottom=value)
with pytest.raises(ValueError):
plt.ylim(top=value)
Going two steps further, you could parametrize the limit functions and sides, but that might be going a bit too far:
@pytest.mark.parametrize('value', [np.inf, np.nan])
@pytest.mark.parametrize(('setter', 'side'), [
(plt.xlim, 'left'),
(plt.xlim, 'right'),
(plt.ylim, 'bottom'),
(plt.ylim, 'top'),
])
def test_invalid_axes_limits(setter, side, value)
limit = {side: value}
with pytest.raises(ValueError):
setter(**limit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might have the 'side' and 'setter' swapped in the parametrize
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuLogic yup I'll fix
Going back a few commits, using Also, please rebase instead of merging. |
#7744 fixed the bug. Can this PR be re-based and sort out if the tests should be expanded? |
As @phobson suggested, i have parametrized the tests and also moved the repeated code inside a function. |
|
||
@pytest.mark.parametrize('value', [np.inf, np.nan]) | ||
@pytest.mark.parametrize(('setter', 'side'), [ | ||
(plt.figure().add_subplot(111, projection='3d').set_xlim3d, 'left'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleanup will work on the test, but I don't think it will work right on the parametrization. Consequently, I don't think it makes sense to create the figure in this list. It should be created in the test itself and only the setter name should be here. You can use getattr
to dynamically access the method you want.
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
@@ -604,8 +615,10 @@ def set_xlim3d(self, left=None, right=None, emit=True, auto=False, **kw): | |||
|
|||
self._process_unit_info(xdata=(left, right)) | |||
if left is not None: | |||
self._validate_axis_limits(left) | |||
left = self.convert_xunits(left) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still recommend moving the convert_xunits
into your _validate...
method
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
if (isinstance(limit, float) and | ||
(not np.isreal(limit) or not np.isfinite(limit))): | ||
raise ValueError("NaN or Inf cannot be the argument values") | ||
return convert(limit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to do the conversion first in case that causes it to be invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor corner comment, but not willing to hold the PR over it.
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
@@ -585,6 +585,19 @@ def _determine_lims(self, xmin=None, xmax=None, *args, **kwargs): | |||
xmax += 0.05 | |||
return (xmin, xmax) | |||
|
|||
def _validate_axis_limits(self, limit, convert): | |||
""" | |||
If the axis limits being set are infinite, this function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First line should be a full sentence; can be shortened to "Raise if specified axis limits are infinite."
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
if limit is not None: | ||
if (isinstance(limit, float) and | ||
(not np.isreal(limit) or not np.isfinite(limit))): | ||
raise ValueError("NaN or Inf cannot be the argument values") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users aren't going to know what "argument values" means; should say "Axis limits cannot be ..."
with pytest.raises(ValueError): | ||
fig = plt.figure() | ||
obj = fig.add_subplot(111, projection='3d') | ||
getattr(obj, setter)(**limit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only this line should be in the context, just in case the first two raise other things accidentally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but it needs a rebase.
@QuLogic @tacaswell made the suggested changes and rebased |
One really minor thing, but I think the message on the first commit might have been messed up. It's talking about the tests and a cleaner version of your work, which seems like a message for a second (now squashed) commit, and maybe the first commit message was lost? |
@QuLogic updated the commit messages |
Refactored and tests added as requested.
if limit is not None: | ||
converted_limits = convert(limit) | ||
if (isinstance(limit, float) and | ||
(not np.isreal(limit) or not np.isfinite(limit))): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh bother, I just noticed this should probably be checking the converted limits. Do you want to open a new PR for that, @nvedant07?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure , i have opened a new PR for this - #8474
Fixed issue #7460