Skip to content

Simplify units.Registry.get_converter. #9314

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 28, 2019
Merged

Simplify units.Registry.get_converter. #9314

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
100 changes: 30 additions & 70 deletions lib/matplotlib/units.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,10 @@ class ConversionError(TypeError):

class AxisInfo(object):
"""
Information to support default axis labeling, tick labeling, and
default limits. An instance of this class must be returned by
:meth:`ConversionInterface.axisinfo`.
Information to support default axis labeling, tick labeling, and limits.

An instance of this class must be returned by
`ConversionInterface.axisinfo`.
"""
def __init__(self, majloc=None, minloc=None,
majfmt=None, minfmt=None, label=None,
Expand Down Expand Up @@ -96,8 +97,7 @@ class ConversionInterface(object):
@staticmethod
def axisinfo(unit, axis):
"""
Return an `~units.AxisInfo` instance for the axis with the
specified units.
Return an `~units.AxisInfo` for the axis with the specified units.
"""
return None

Expand All @@ -112,19 +112,19 @@ def default_units(x, axis):
def convert(obj, unit, axis):
"""
Convert *obj* using *unit* for the specified *axis*.
If *obj* is a sequence, return the converted sequence.
The output must be a sequence of scalars that can be used by the numpy
array layer.

If *obj* is a sequence, return the converted sequence. The output must
be a sequence of scalars that can be used by the numpy array layer.
"""
return obj

@staticmethod
def is_numlike(x):
"""
The Matplotlib datalim, autoscaling, locators etc work with
scalars which are the units converted to floats given the
current unit. The converter may be passed these floats, or
arrays of them, even when units are set.
The Matplotlib datalim, autoscaling, locators etc work with scalars
which are the units converted to floats given the current unit. The
converter may be passed these floats, or arrays of them, even when
units are set.
"""
if np.iterable(x):
for thisx in x:
Expand All @@ -134,73 +134,33 @@ def is_numlike(x):


class Registry(dict):
"""
A register that maps types to conversion interfaces.
"""
def __init__(self):
dict.__init__(self)
self._cached = {}
"""Register types with conversion interface."""

def get_converter(self, x):
"""
Get the converter for data that has the same type as *x*. If no
converters are registered for *x*, returns ``None``.
"""

if not len(self):
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to keep this in, to avoid stepping through the following logic if there aren't any convertors registered in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the cost of an instance check, a dict lookup and attempting to get the first element is quite small. For code like this I would like to see at least a microbenchmark showing at least some minor improvement (not necessarily a big one) before making it more complex than necessary.

return None # nothing registered
# DISABLED idx = id(x)
# DISABLED cached = self._cached.get(idx)
# DISABLED if cached is not None: return cached

converter = None
classx = getattr(x, '__class__', None)

if classx is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also stay in, as there's no guarantee that unit data is a subclass of numpy array or an iterable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be handled by return self[type(x)], no?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes

converter = self.get(classx)

if converter is None and hasattr(x, "values"):
# this unpacks pandas series or dataframes...
x = x.values

# If x is an array, look inside the array for data with units
"""Get the converter interface instance for *x*, or None."""
if hasattr(x, "values"):
Copy link
Member

Choose a reason for hiding this comment

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

@ImportanceOfBeingErnest had some concerns about this way to check for pandas:
#11664 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One could do something like type(x).__module__.startswith("pandas.") and hasattr(x, "values") but that should be a separate PR; this PR doesn't change the way pandas testing is done.

x = x.values # Unpack pandas Series and DataFrames.
if isinstance(x, np.ndarray):
# In case x in a masked array, access the underlying data (only its
# type matters). If x is a regular ndarray, getdata() just returns
# the array itself.
x = np.ma.getdata(x).ravel()
# If there are no elements in x, infer the units from its dtype
if not x.size:
return self.get_converter(np.array([0], dtype=x.dtype))
xravel = x.ravel()
try:
# pass the first value of x that is not masked back to
# get_converter
if not np.all(xravel.mask):
# Get first non-masked item
converter = self.get_converter(
xravel[np.argmin(xravel.mask)])
return converter
except AttributeError:
# not a masked_array
# Make sure we don't recurse forever -- it's possible for
# ndarray subclasses to continue to return subclasses and
# not ever return a non-subclass for a single element.
next_item = xravel[0]
if (not isinstance(next_item, np.ndarray) or
next_item.shape != x.shape):
converter = self.get_converter(next_item)
return converter

# If we haven't found a converter yet, try to get the first element
if converter is None:
try:
thisx = cbook.safe_first_element(x)
try: # Look up in the cache.
return self[type(x)]
except KeyError:
try: # If cache lookup fails, look up based on first element...
first = cbook.safe_first_element(x)
except (TypeError, StopIteration):
pass
else:
if classx and classx != getattr(thisx, '__class__', None):
converter = self.get_converter(thisx)
return converter

# DISABLED self._cached[idx] = converter
return converter
# ... and avoid infinite recursion for pathological iterables
# where indexing returns instances of the same iterable class.
if type(first) is not type(x):
return self.get_converter(first)
return None


registry = Registry()