Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
FIX: show bars when the first location is nan
Due to the way we handle units on the bar width having an invalid value in the
first position of the x bar (of y of barh) would effectively poison all of the
widths making all of the bars invisible.

This also renames the cbook function _safe_first_non_none function ->
_safe_first_finite and adjusts the behavior to also drop nans

closes #23687
  • Loading branch information
tacaswell committed Aug 26, 2022
commit cfb27b348139f635325734ff4509e6d67a4b6e54
14 changes: 7 additions & 7 deletions lib/matplotlib/axes/_axes.py
Original file line number Diff line number Diff line change
Expand Up @@ -2179,12 +2179,12 @@ def _convert_dx(dx, x0, xconv, convert):
# removes the units from unit packages like `pint` that
# wrap numpy arrays.
try:
x0 = cbook._safe_first_non_none(x0)
x0 = cbook._safe_first_finite(x0)
except (TypeError, IndexError, KeyError):
pass

try:
x = cbook._safe_first_non_none(xconv)
x = cbook._safe_first_finite(xconv)
except (TypeError, IndexError, KeyError):
x = xconv

Expand Down Expand Up @@ -2801,11 +2801,11 @@ def broken_barh(self, xranges, yrange, **kwargs):
"""
# process the unit information
if len(xranges):
xdata = cbook._safe_first_non_none(xranges)
xdata = cbook._safe_first_finite(xranges)
else:
xdata = None
if len(yrange):
ydata = cbook._safe_first_non_none(yrange)
ydata = cbook._safe_first_finite(yrange)
else:
ydata = None
self._process_unit_info(
Expand Down Expand Up @@ -3447,10 +3447,10 @@ def _upcast_err(err):
# safe_first_element because getitem is index-first not
# location first on pandas objects so err[0] almost always
# fails.
isinstance(cbook._safe_first_non_none(err), np.ndarray)
isinstance(cbook._safe_first_finite(err), np.ndarray)
):
# Get the type of the first element
atype = type(cbook._safe_first_non_none(err))
atype = type(cbook._safe_first_finite(err))
# Promote the outer container to match the inner container
if atype is np.ndarray:
# Converts using np.asarray, because data cannot
Expand Down Expand Up @@ -4313,7 +4313,7 @@ def _parse_scatter_color_args(c, edgecolors, kwargs, xsize,
c_is_string_or_strings = (
isinstance(c, str)
or (np.iterable(c) and len(c) > 0
and isinstance(cbook._safe_first_non_none(c), str)))
and isinstance(cbook._safe_first_finite(c), str)))

def invalid_shape_exception(csize, xsize):
return ValueError(
Expand Down
19 changes: 15 additions & 4 deletions lib/matplotlib/cbook/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1703,10 +1703,10 @@ def safe_first_element(obj):
This is an type-independent way of obtaining the first element,
supporting both index access and the iterator protocol.
"""
return _safe_first_non_none(obj, skip_none=False)
return _safe_first_finite(obj, skip_nonfinite=False)


def _safe_first_non_none(obj, skip_none=True):
def _safe_first_finite(obj, *, skip_nonfinite=True):
"""
Return the first non-None element in *obj*.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Docstring needs to be adapted.

This is a method for internal use.
Expand All @@ -1715,7 +1715,14 @@ def _safe_first_non_none(obj, skip_none=True):
supporting both index access and the iterator protocol.
The first non-None element will be obtained when skip_none is True.
"""
if skip_none is False:
def safe_isfinite(val):
try:
return np.isfinite(val) if np.isscalar(val) else True
Comment thread
timhoffm marked this conversation as resolved.
except TypeError:
# This is something that numpy can not make heads or tails
# of, assume "finite"
return True
if skip_nonfinite is False:
if isinstance(obj, collections.abc.Iterator):
# needed to accept `array.flat` as input.
# np.flatiter reports as an instance of collections.Iterator
Expand All @@ -1730,12 +1737,16 @@ def _safe_first_non_none(obj, skip_none=True):
"as input")
return next(iter(obj))
elif isinstance(obj, np.flatiter):
# TODO do the finite filtering on this
return obj[0]
elif isinstance(obj, collections.abc.Iterator):
raise RuntimeError("matplotlib does not "
"support generators as input")
else:
return next(val for val in obj if val is not None)
return next(
val for val in obj
if val is not None and safe_isfinite(val)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Logically, should we absorb the None check in safe_isfinite? -I think None is not finite.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we need to check this here:

In [62]: np.isfinite(None)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In [63], line 1
----> 1 np.isfinite(None)

TypeError: ufunc 'isfinite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, we need the check. I was suggesting that it could logically belong inside safe_isfinite (by the argument that None is not a finite number, similar to NaN).

i.e.

def safe_isfinite(val):
    if val is None:
        return False
    ...

and here next(val for val in obj if safe_isfinite(val))

I just realized that this is an inner function, so it's not too important, but I'd still have a slight preference. I'll approve and you can decide whether you want to change it or leave it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree that is nicer.

)


def sanitize_sequence(data):
Expand Down
2 changes: 1 addition & 1 deletion lib/matplotlib/dates.py
Original file line number Diff line number Diff line change
Expand Up @@ -1908,7 +1908,7 @@ def default_units(x, axis):
x = x.ravel()

try:
x = cbook._safe_first_non_none(x)
x = cbook._safe_first_finite(x)
except (TypeError, StopIteration):
pass

Expand Down
24 changes: 24 additions & 0 deletions lib/matplotlib/tests/test_axes.py
Original file line number Diff line number Diff line change
Expand Up @@ -8109,3 +8109,27 @@ def test_get_xticklabel():
for ind in range(10):
assert ax.get_xticklabels()[ind].get_text() == f'{ind}'
assert ax.get_yticklabels()[ind].get_text() == f'{ind}'


def test_bar_leading_nan():

barx = np.arange(3, dtype=float)
barheights = np.array([0.5, 1.5, 2.0])
barstarts = np.array([0.77]*3)

barx[0] = np.NaN

fig, ax = plt.subplots()

bars = ax.bar(barx, barheights, bottom=barstarts)

hbars = ax.barh(barx, barheights, left=barstarts)

for bar_set in (bars, hbars):
# the first bar should have a nan in the location
nanful, *rest = bar_set
assert (~np.isfinite(nanful.xy)).any()
assert np.isfinite(nanful.get_width())
for b in rest:
assert np.isfinite(b.xy).all()
assert np.isfinite(b.get_width())
6 changes: 3 additions & 3 deletions lib/matplotlib/tests/test_cbook.py
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ def test_flatiter():
it = x.flat
assert 0 == next(it)
assert 1 == next(it)
ret = cbook._safe_first_non_none(it)
ret = cbook._safe_first_finite(it)
assert ret == 0

assert 0 == next(it)
Expand Down Expand Up @@ -758,7 +758,7 @@ def test_contiguous_regions():
def test_safe_first_element_pandas_series(pd):
# deliberately create a pandas series with index not starting from 0
s = pd.Series(range(5), index=range(10, 15))
actual = cbook._safe_first_non_none(s)
actual = cbook._safe_first_finite(s)
assert actual == 0


Expand Down Expand Up @@ -893,5 +893,5 @@ def test_format_approx():
def test_safe_first_element_with_none():
datetime_lst = [date.today() + timedelta(days=i) for i in range(10)]
datetime_lst[0] = None
actual = cbook._safe_first_non_none(datetime_lst)
actual = cbook._safe_first_finite(datetime_lst)
assert actual is not None and actual == datetime_lst[1]
2 changes: 1 addition & 1 deletion lib/matplotlib/units.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ def get_converter(self, x):
except KeyError:
pass
try: # If cache lookup fails, look up based on first element...
first = cbook._safe_first_non_none(x)
first = cbook._safe_first_finite(x)
except (TypeError, StopIteration):
pass
else:
Expand Down