Skip to content
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

Check for float values for min/max values to ax{v,h}line #17822

Merged
merged 2 commits into from Jul 27, 2020

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Jul 2, 2020

Fixes #12198. This now errors if a non-float value is provided for xmin/xmax or ymin/ymax to ax{v,h}{line,span}. This should get an API change or a what's new, but I'm not sure which, could someone advise?

@tacaswell
Copy link
Member

I don't think checking for the ability to cast to float is enough, we need to make sure that the data will not select a converter.

@dstansby
Copy link
Member Author

I found matplotlib.units._is_natively_supported, which I think does the correct job here.

@jklymak
Copy link
Member

jklymak commented Jul 13, 2020

On master

import matplotlib.pyplot as plt
import matplotlib.dates as mdates
line = plt.axvline(
    x=.5, ymin=mdates.num2date(1), ymax=mdates.num2date(1.05))
line.set_clip_on(False)

plt.show()

already errors out:

Traceback (most recent call last):
  File "./testAXvline.py", line 3, in <module>
    line = plt.axvline(
  File "/Users/jklymak/matplotlib/lib/matplotlib/pyplot.py", line 2464, in axvline
    return gca().axvline(x=x, ymin=ymin, ymax=ymax, **kwargs)
  File "/Users/jklymak/matplotlib/lib/matplotlib/axes/_axes.py", line 917, in axvline
    self.add_line(l)
  File "/Users/jklymak/matplotlib/lib/matplotlib/axes/_base.py", line 1977, in add_line
    self._update_line_limits(line)
  File "/Users/jklymak/matplotlib/lib/matplotlib/axes/_base.py", line 1999, in _update_line_limits
    path = line.get_path()
  File "/Users/jklymak/matplotlib/lib/matplotlib/lines.py", line 1011, in get_path
    self.recache()
  File "/Users/jklymak/matplotlib/lib/matplotlib/lines.py", line 658, in recache
    y = _to_unmasked_float_array(yconv).ravel()
  File "/Users/jklymak/matplotlib/lib/matplotlib/cbook/__init__.py", line 1298, in _to_unmasked_float_array
    return np.asarray(x, float)
  File "/Users/jklymak/anaconda3/envs/matplotlibdev/lib/python3.8/site-packages/numpy/core/_asarray.py", line 85, in asarray
    return array(a, dtype, copy=False, order=order)
TypeError: float() argument must be a string or a number, not 'datetime.datetime'
(matplotlibdev)

So is the point of this PR to just make the error more explicit? If so, I don't think it needs an API change or note, because I guess that has already occurred.

@QuLogic
Copy link
Member

QuLogic commented Jul 14, 2020

That specific example broke in a793bde, which is a date-specific change. Is it still true that other unit-ful values are currently broken?

@dstansby
Copy link
Member Author

Yes, I think it might just be a better error now, with the following example

import matplotlib.pyplot as plt
import astropy.units as u
from astropy.visualization import quantity_support
quantity_support()

fig, ax = plt.subplots()
ax.scatter(1 * u.m, 1 * u.s)
ax.axhline(1 * u.s, xmin=0.1 * u.m)
plt.show()

The error before was

Traceback (most recent call last):
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/axis.py", line 1524, in convert_units
    ret = self.converter.convert(x, self.units, self)
  File "/Users/dstansby/github/astropy/astropy/visualization/units.py", line 102, in convert
    return [v.to_value(unit) for v in val]
  File "/Users/dstansby/github/astropy/astropy/visualization/units.py", line 102, in <listcomp>
    return [v.to_value(unit) for v in val]
AttributeError: 'int' object has no attribute 'to_value'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "test.py", line 8, in <module>
    ax.axhline(1 * u.s, xmin=0.1 * u.m)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/axes/_axes.py", line 847, in axhline
    self.add_line(l)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/axes/_base.py", line 1977, in add_line
    self._update_line_limits(line)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/axes/_base.py", line 1999, in _update_line_limits
    path = line.get_path()
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/lines.py", line 1011, in get_path
    self.recache()
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/lines.py", line 652, in recache
    xconv = self.convert_xunits(self._xorig)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/artist.py", line 175, in convert_xunits
    return ax.xaxis.convert_units(x)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/axis.py", line 1526, in convert_units
    raise munits.ConversionError('Failed to convert value(s) to axis '
matplotlib.units.ConversionError: Failed to convert value(s) to axis units: [<Quantity 0.1 m>, 1]

And the error with this PR is

Traceback (most recent call last):
  File "test.py", line 8, in <module>
    ax.axhline(1 * u.s, xmin=0.1 * u.m)
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/axes/_axes.py", line 834, in axhline
    self._check_no_units([xmin, xmax], ['xmin', 'xmax'])
  File "/Users/dstansby/github/matplotlib/lib/matplotlib/axes/_axes.py", line 926, in _check_no_units
    raise ValueError(f"{name} must be a single scalar value, "
ValueError: xmin must be a single scalar value, but got 0.1 m

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

I think this is an improved error message.

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@timhoffm timhoffm merged commit 7b574c3 into matplotlib:master Jul 27, 2020
@dstansby dstansby deleted the float-check branch July 27, 2020 17:19
@QuLogic
Copy link
Member

QuLogic commented Jul 28, 2020

Hmm, those exception lines are too long; I wonder why flake8 is not complaining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

axvline incorrectly tries to handle unitized ymin, ymax
5 participants