API: check locator and formatter args when passed

[jklymak]

PR Summary

Right now, ax.set_major_formatter and ax.set_major_locator throw pretty deep error messages if the wrong type of data is passed to them. This catches at entry.

Closes #10769

import matplotlib
import matplotlib.pyplot as plt

f, ax = plt.subplots(1, 1, figsize=(15, 10))
plt.plot([10**4, 11*10**4], [100, 150])
ax.xaxis.set_major_formatter(matplotlib.ticker.LogLocator())
plt.show()

Now returns:

Traceback (most recent call last):
  File "testEngTicker.py", line 6, in <module>
    ax.xaxis.set_major_formatter(matplotlib.ticker.LogLocator())
  File "/Users/jklymak/matplotlib/lib/matplotlib/axis.py", line 1573, in set_major_formatter
    "formatter argument must be matplotlib.ticker.Formatter")
ValueError: formatter argument must be matplotlib.ticker.Formatter

instead of something down in the bowels of drawing the ticker...

PR Checklist

• Code is PEP 8 compliant

[afvincent]

@jklymak For what reason did you labelled it with the “API consistency” tag?

[jklymak]

Umm, because there is no argument-checking flag.

[jklymak]

There!

[afvincent]

Well it was more by curiosity that I was asking ^^. The docstrings seemed to be pretty clear about the kind of expected inputs and your changes is simply enforcing that information, is it not ;)?

[jklymak]

Well, the documentation is specifying the API 😉

[afvincent]

LGTM to me :)! (Approval modulo the tests are passing of course)

One could argue that the exception messages might be made even more “precise”, in the sense for example that the “formatter argument must be an instance of matplotlib.ticker.Formatter or of another derived class”. But IMO, even in its current state, this PR is a definitively “good enough©” improvement compared to the former situation...

[anntzer]

I don't really like the change, in general we are (and Python is) pretty lax in accepting anything that duck types correctly, and just let things blow up if the argument has the wrong type.

Regarding the error message (if this goes in): an instance of a subclass of Ticker is also an instance of Ticker (literally, as returned by isinstance...) so I wouldn't try to make it longer.

[jklymak]

Well, I'm certainly willing to go by convention, but the un-patched error is in draw(), so its a bit hopeless for the uninitiated to know that the error was actually in set_major_formatter:

Traceback (most recent call last):
  File "/Users/jklymak/matplotlib/lib/matplotlib/backends/backend_qt5.py", line 520, in _draw_idle
    self.draw()
  File "/Users/jklymak/matplotlib/lib/matplotlib/backends/backend_agg.py", line 426, in draw
    self.figure.draw(self.renderer)
  File "/Users/jklymak/matplotlib/lib/matplotlib/artist.py", line 55, in draw_wrapper
    return draw(artist, renderer, *args, **kwargs)
  File "/Users/jklymak/matplotlib/lib/matplotlib/figure.py", line 1504, in draw
    renderer, self, artists, self.suppressComposite)
  File "/Users/jklymak/matplotlib/lib/matplotlib/image.py", line 139, in _draw_list_compositing_images
    a.draw(renderer)
  File "/Users/jklymak/matplotlib/lib/matplotlib/artist.py", line 55, in draw_wrapper
    return draw(artist, renderer, *args, **kwargs)
  File "/Users/jklymak/matplotlib/lib/matplotlib/axes/_base.py", line 2588, in draw
    mimage._draw_list_compositing_images(renderer, self, artists)
  File "/Users/jklymak/matplotlib/lib/matplotlib/image.py", line 139, in _draw_list_compositing_images
    a.draw(renderer)
  File "/Users/jklymak/matplotlib/lib/matplotlib/artist.py", line 55, in draw_wrapper
    return draw(artist, renderer, *args, **kwargs)
  File "/Users/jklymak/matplotlib/lib/matplotlib/axis.py", line 1189, in draw
    ticks_to_draw = self._update_ticks(renderer)
  File "/Users/jklymak/matplotlib/lib/matplotlib/axis.py", line 1027, in _update_ticks
    tick_tups = list(self.iter_ticks())  # iter_ticks calls the locator
  File "/Users/jklymak/matplotlib/lib/matplotlib/axis.py", line 973, in iter_ticks
    self.major.formatter.set_locs(majorLocs)
AttributeError: 'LogLocator' object has no attribute 'set_locs'
Traceback (most recent call last):
  File "/Users/jklymak/matplotlib/lib/matplotlib/axes/_base.py", line 3627, in format_xdata
    return self.fmt_xdata(x)
TypeError: 'NoneType' object is not callable

Worse, if you happen to drag your mouse across the figure window, the same error is emitted repeatedly in each draw attempt.

[jklymak]

If it looks like this will get approved, I'll make some tests so codecov is happy. But I'm not advocating strongly that it should be merged if its really atypical to have this type of argument checking...

[anntzer]

(I'm not trying to block the change, but I think it's worth getting some more opinions...)

[story645]

@jklymak I'm +1 on this if it has tests and with @afvincent suggestion of using 'is instance of" type language because I want this stuff to be more common in the codebase.

[jklymak]

Added tests.... happy to be tutored on making these parameterized...

[efiring]

The cost of putting in these checks is low; they are simple and fast. I'm not in favor of checking all inputs at every stage, but this looks like a case where the cost/benefit ratio is favorable, so I lean towards accepting this PR.

[timhoffm]

To support duck typing, Python code tries to avoid isinstance(obj, cls) in favor of hasattr(obj, name). Not sure about the canonical attributes or methods of locators and formatters, however, I would suggest checks like if not hasattr(formatter, 'format_data') and if not hasattr(locator, 'tick_values')

[jklymak]

Does that become an AttributeError in that case?

[jklymak]

eh, and probably should have been TypeError if not and AttributeError. Certainly ValueError is incorrect?

[timhoffm]

I'd go for TypeError. By intent, the hasattr here is a kind of a loose type checking.

[timhoffm]

Parametrizing would be something like:

@pytest.mark.parametrize('setter', [
    ax.xaxis.set_minor_formatter,
    ax.xaxis.set_major_formatter,
])
def test_set_formatter_type(setter):
    fig, ax = plt.subplots()
    with pytest.raises(ValueError):
        setter(matplotlib.ticker.LogFormatter())

or

@pytest.mark.parametrize('setter,cls', [
    (ax.xaxis.set_minor_formatter, matplotlib.ticker.LogFormatter),
    (ax.xaxis.set_major_formatter, matplotlib.ticker.LogFormatter),
    (ax.xaxis.set_minor_locator, matplotlib.ticker.LogLocator),
    (ax.xaxis.set_major_locator, matplotlib.ticker.LogLocator),
])
def test_set_formatter_or_locator_type(setter, cls):
    fig, ax = plt.subplots()
    with pytest.raises(ValueError):
        setter(cls())

However, I think this is not readable and I'd leave the four single tests.

[timhoffm]

one could shorten this to

raise TypeError("formatter must be a matplotlib.ticker.Formatter")

But that's a matter of taste. I'll leave this up to you.

[jklymak]

Thats what I had before but folks wanted "instance of" 😉

[efiring]

I'm not asking for any more changes--it's good enough as-is--but I disagree with the switch to duck-typing here. Duck-typing is a useful technique, but it is not always more appropriate than checking for membership in a class. In the particular case here, there is no fundamental reason a Formatter needs a format_data method, although our base class has one; and the normal way of using the Formatter instance is via its 'call', so someone could easily write a "Formatter" that would pass the quack-test used here but fail to work in practice. Or one that would fail the quack test but would in fact work just fine. It is much less likely that a custom Formatter subclassed from our base would fail. And the docstring specifies a Formatter, so testing for that is more consistent than quack-testing.

(It looks like we don't use the 'format_data' method anywhere except in the jpl_units test...)

[jklymak]

@efiring That actually makes sense to me. It'd be pretty strange situation where someone bothered to pass a formatter or locator in that was not a subclass... Further, its certainly bad to test on a method thats not used (format_data) or one thats too generic (call). I'l wait a bit, maybe @timhoffm has a counter argument, but I'll make the suggested change before committing if not...

[story645]

While I'm with @timhoffm that I don't think it actually needs to be parameterized, pushing his example more:

@pytest.mark.parametrize('setter', [ax.xaxis.set_minor_formatter, ax.xaxis.set_major_formatter])
@pytest.mark.parametrize('cls', [matplotlib.ticker.LogFormatter, matplotlib.ticker.LogLocator])
def test_set_formatter_or_locator_type(setter, cls):
    fig, ax = plt.subplots()
    with pytest.raises(ValueError):
        setter(cls())

and agree with @efiring on isinstance.

[timhoffm]

Should this be backported?

[efiring]

The intention is to restrict backports to genuine bug-fixes, and soon only to critical bug-fixes, so this is not a candidate for backporting.