Just for symmetry, please move this down to # Test image-

Thank you @timhoffm for advice. Have moved it down.

Please move down as well.

Have moved it down.

I'm not sure about the design here. On the one hand, needing a converter around the converter feels wrong. On the other hand, do we want to change the return type of convert_xunits()?

I agree that the design seems(?) problematic (is this converter going to end up being needed everywhere?).

Thanks for advice @timhoffm @anntzer . But I am not sure whether I should update convert_units(self, x) method in Axis directly.

convert_xunits mainly consumes convert_units(self, x) in Axis class, whose first line is:

# If x is already a number, doesn't need converting
if munits.ConversionInterface.is_numlike(x):
      return x

Since Decimal is actually a Number, this method doesn't change the input x if x is a list of Decimal.

One possible solution is like:

# If x is already a number, doesn't need converting
if munits.ConversionInterface.is_numlike(x):
      return np.asarray(x, dtype=np.float)

But I am afraid this way may cause side effect when input x is a list of integer or other number type.

If we check whether x contains Decimal to decide whether need to convert x, then I feel the is_numlike(x) function in ConversionInterface doesn't quite make sense here.

Besides, I am curious about the following code in is_numlike(x):

if iterable(x):
    for thisx in x:
        return isinstance(thisx, Number)

This only check the first element in a list. Is this what it supposed to do when input x is a list?

Perhaps is_numlike shouldbe changed to isinstance(x, Number) and not isinstance(x, Decimal) then (dunno).
The unit system assumes that lists are homogeneous. Yes, it's a pretty awkward assumption.

I doubt the precision matters as everything ultimately gets converted to float at some point.

The default converter would be implemented in units.py but I certainly haven't thought about the specifics...

I haven't spent a lot of time to think about it, but perhaps the correct approach may be to remove the if is_numlike(x): return x in convert_units() and instead always go through the unit conversion system, and then add a converter for Number (which will thus be used both for e.g. floats and for Decimal) which casts the input to float.

On first thought, it sounds reasonable to not special-case numbers but just handle them as a united quantity. However, I'm not sure if that would work:

    def convert_units(self, x):
        # If x is already a number, doesn't need converting
        if munits.ConversionInterface.is_numlike(x):
            return x

        if self.converter is None:
            self.converter = munits.registry.get_converter(x)

If self.converter is not None, you wouldn't get the proposed NumberFormatter; and therefore all converters would be required to pass numbers through`.

@sasoripathos Sorry that we're quite slow and vague with responses here. You've chosen a tricky issue; and as you can see from the discussion, we do not have a clear idea what the appropriate fix should be.

@timhoffm Thanks for reply! That's also a place I am confused about. I still haven't figure out what should be the return value of axisinfo(unit, axis) and default_units(x, axis) if i implemented the proposed NumberFormatter. Please pardon my beginner knowledge about matplotlib. I am still looking into the proposed approach and will give a try next week. I also think my current implementation is not that elegent since it (kind of) "breaks" the Number type, but i think it may be (hopefully) a working walk around.

@timhoffm @anntzer @jklymak Hi! I just pushed a DecimalConverter I implemented. Since originally no convertion will be done if x is a number-like input, I just make axisinfo(unit, axis) return empty AxisInfo and default_units(x, axis) return None. Besides, I use ConversionInterface.is_numlike(x) to avoid requiring all existing converters pass Numbers.

Could you please have a review and give me some feedback? Thanks very much!

I don't think testing both bar and barh is particularly necessary and would just keep one of them.

what happens if width (or height) is a Decimal?

I think there will be no problem since width will also be converted. In the following code, if width is of type Decimal, it will be converted to floats by convert_xunits in _convert_dx eventually.

 if self.xaxis is not None:
    x0 = x
    x = np.asarray(self.convert_xunits(x))
    width = self._convert_dx(width, x0, x, self.convert_xunits)
    ......

Similarly for height.

I can generate plots with float x/y but Decimal width/height as this:

Can be left out. The following code already states exactly that.

I would change this to

else:
    # assume x is a list of Decimal

Is it really necessary this way? The purpose of converters is to convert to something number-like matplotlib can work with. So the original logic "if is number return else convert" seems more clear than "convert; if that fails, did we already have a number? Ok then we just use it".

Maybe munits.ConversionInterface.is_numlike() is just not the right function/name. I'd prefer this to stay and be

if not munits.ConversionInterface.needs_conversion(x):
    return x

That will allow to cleanly pass through Decimals.

Well, not sure on the sign of the logic is_natively_supported_numlike(x) may be better than not needs_conversion(x).

The original cause of this problem is the operations like plus between Decimal and float is not supported by matplotlib. If there is not such operations in the logic, Decimal input will not cause any error. As we discussed before, it doesn't quite make sense to distinguish Decimal and other types of number since Decimal is a type of Number.

That's why I'm saying "is_numlike" is not the right check. Semantically, we want

if matplotlib_can_natively_work_with(x):
     return x
# try to convert x

It's not about number or not, it's about "do we need a converter". So, if matplotlib cannot natively handle Decimal that can fall through to the converter.

How can we define natively handle? For Decimal input, matplotlib supports it if there is no operation between Decimal and float. For example:

x = [1, 2, 3, 4]
y = [Decimal(1.5), Decimal(2.5), Decimal(3.5), Decimal(4.5)]
ax.bar(x, y, align='center')

This piece of code works fine. I am not sure about other kinds of plot, but based on this, I think matplotlib does not totally support/unsupport Decimal.

Well, operationally, "can natively handle" is currently is_numlike(x) and not isinstance(x, Decimal). I mean, that's what your proposed code does as well.

It's a bit clumsy to define is that way, but that's the current logic. And I prefer to have that logic explicitly written down in a single place in a function.

I see currently is_numlike is used in some places like StrCategoryConverter.convert() in category.py. And from the code (as below), it looks like the Decimal input will not cause any problems here (since Decimal will be converted to floats).

# pass through sequence of non binary numbers
if all((units.ConversionInterface.is_numlike(v) and
        not isinstance(v, (str, bytes))) for v in values):
    return np.asarray(values, dtype=float)

So I think maybe instead of change is_numlike(x), which cause API changes, we can add a new function is_natively_supported_numlike(x) like proposed in ConversionInterface. What do you think?

I think it's a good idea to leave is_numlike(x) as is and introduce and use ConversionInterface.is_natively_supported_numlike(x)

Naming: Not sure if we need the _numlike suffix in the name. On the one hand, this is exactly meant as is_natively_supported - no artificial restriction to numlikes necessary (even though it's unlikely that we'll natively support non-numlikes). On the other hand, the _numlike may give additional context to the reader, but I would see this more as a job of the is_natively_supported* docstring.

is_numlike(x) may become deprecated in the future, but we can handle this later.

I have added a function named is_natively_supported() in ConversionInterface. Since the current unit system assume lists are homogeneous, I followed the same style as is_numlik(). Now I only consider all numbers except Decimal are natively supported.

what happens if width (or height) is a Decimal?