Keep explicit ticklabels in sync with ticks from FixedLocator #17266

efiring · 2020-04-29T23:23:30Z

PR Summary

Using FixedFormatter with ticks from FixedLocator causes mislabeled ticks if the nbins kwarg of the locator triggers subsampling. This PR addresses that in the common case where the Axis.set_ticklabels method is used, by setting a FuncFormatter instead of a FixedFormatter.

Closes #11937.

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

When explicitly setting the tick locations and then the corresponding tick labels, a FixedLocator is used for the locations. It has an nbins argument that can trigger subsampling. Previously, tick labels were made by FixedFormatter and based on position, not value, so subsampling caused a mismatch, yielding incorrect tick labels. This is changed to use FuncFormatter with a function to look up the label by value, ignoring the position.

QuLogic · 2020-04-30T04:37:34Z

xref #15005

anntzer · 2020-04-30T08:21:46Z

lib/matplotlib/axis.py

            ticks = self.get_major_ticks()
+
+        self._update_ticks()


do you really need to call update_ticks() here? and if you do, shouldn't this go before get_major_ticks()/get_minor_ticks()? (otherwise you'd be possibly invalidating ticks even before iterating on them...)

anntzer · 2020-04-30T08:23:50Z

lib/matplotlib/ticker.py

@@ -379,18 +379,28 @@ class FuncFormatter(Formatter):

    The function should take in two inputs (a tick value ``x`` and a
    position ``pos``), and return a string containing the corresponding
-    tick label.
+    tick label.  The function may take additional arguments that are
+    supplied upon instantiation.


"appended" (they go after x, pos, not before).

anntzer · 2020-04-30T08:26:42Z

lib/matplotlib/axis.py

+                   else self.get_major_locator())
+        if isinstance(locator, mticker.FixedLocator):
+            tickd = {loc: lab for loc, lab in zip(locator.locs, ticklabels)}
+            formatter = mticker.FuncFormatter(self._format_with_dict, tickd)


I'm not sure whether we want or not to make this extension to the FuncFormatter API (perhaps?) but if not, I think you can get away with

@staticmethod def _format_with_ticks(tickd, x, pos): ... # move tickd to first ... formatter = FuncFormatter(functools.partial(self._format_with_ticks, tickd))

as the partial object will stay picklable (I think this is more or less the canonical way to bind some arguments while keeping picklability).

efiring · 2020-04-30T21:58:46Z

@anntzer I think I have addressed all your points. I had tried using partial earlier, but hadn't gotten it right. _update_ticks is overkill, but some of its function is essential to passing the pre-existing test_axes.py::test_set_get_ticklabels, so I put the needed parts directly into set_ticklabels.

anntzer · 2020-04-30T22:03:03Z

lib/matplotlib/axis.py

-        for tick_label, tick in zip(ticklabels, ticks):
+        for pos, (loc, tick) in enumerate(zip(locs, ticks)):
+            tick.update_position(loc)
+            tick_label = formatter(tick._loc, pos)


is tick._loc just loc? or not?

At this point they should be identical; but yes, it might look better and be slightly more efficient with loc.

anntzer

just one question, and needs ci to pass

QuLogic · 2020-05-04T20:01:18Z

lib/matplotlib/tests/test_axes.py

+    ax.xaxis.set_ticks(np.arange(10) + 0.1)
+    ax.locator_params(nbins=5)
+    ax.xaxis.set_ticklabels([c for c in "bcdefghijk"])


Putting these together would make it a bit clearer:

Suggested change

ax.xaxis.set_ticks(np.arange(10) + 0.1)

ax.locator_params(nbins=5)

ax.xaxis.set_ticklabels([c for c in "bcdefghijk"])

ax.xaxis.set(ticks=np.arange(10) + 0.1),

ticklabels=[c for c in "bcdefghijk"])

ax.locator_params(nbins=5)

The use of set here would work only by accident, because the kwargs are sorted in inverse order, so that set_ticks will be called before set_ticklabels. I will stick with the individual function calls.

This is the closest analogue to plt.xticks(locs, labels), so if you're saying it might be broken, then I think that's not good. Especially given the recent tweet about .set()...

I don't use twitter and haven't seen the tweet.

I'm not saying the behavior is currently broken, just that it works in this case (I think--I haven't tried it) because the names are sorted in inverse order, and the "s" in "ticks" makes it sort before the first "l" in "ticklabels". It's an accident. It might be better to make it explicit by adding a "ticks" entry to Artist._prop_order to boost its priority. The _prop_order dict provides a way of setting priority at a higher level than the alphanumeric sort. Presently, it serves only to give 'color' low priority, so it is executed last.

The is the tweet that @story645 mentioned.

For users, working by accident and working by design are the same thing. So I meant if this PR breaks it, that would not be good. However, it doesn't, so we're fine, even if it's working by accident right now.

I guess I'll take advantage of this to point to #16328 :)

jklymak · 2020-05-04T20:31:10Z

As discussed on the call, it would be good if there was a lock on set_ticklabels that said it had to be the same length as set_ticks.

jklymak · 2020-05-04T21:18:31Z

lib/matplotlib/axis.py

@@ -1635,6 +1635,11 @@ def set_ticklabels(self, ticklabels, *, minor=False, **kwargs):
        locator = (self.get_minor_locator() if minor
                   else self.get_major_locator())
        if isinstance(locator, mticker.FixedLocator):
+            if len(locator.locs) != len(ticklabels):


L 1620 probably needs something like "Number of labels must be the same as number of ticks set by set_ticks or the FixedLocator"?

I added a reference to set_ticks.

That helps, but I mean above in the Parameter description. Its a bit unclear as to what is meant by "tick labels". maybe:

ticklabels : sequence of str or of `.Text`\s List of texts for tick labels, one for each tick defined in `.axis.set_ticks`.

efiring · 2020-05-04T23:10:07Z

Adding the check for a match turned up a blatant error in our image_annotated_heatmap example. I wonder how many such mismatches are out in the wild, and not as obvious as in our example? We'll see...

efiring · 2020-05-05T02:25:20Z

The appveyor failure appears to be caused by the crash of the VM running the test.

efiring · 2020-05-05T06:29:52Z

@jklymak I have augmented the ticklabels kwarg description in the docstring, and updated a statement in the API docs about set_ticks and set_ticklabels.

jklymak · 2020-05-06T02:49:35Z

I'll leave here since there is a mailing list discussion, but anyone may merge after that is done.

efiring added 5 commits April 28, 2020 15:25

Handle missing values.

e44d74a

Keep ticks and labels in sync; add a test.

433308c

Flake8: space added

775db02

Make pickles work again.

94f04a9

anntzer reviewed Apr 30, 2020

View reviewed changes

anntzer added the topic: ticks axis labels label Apr 30, 2020

anntzer reviewed Apr 30, 2020

View reviewed changes

efiring added 2 commits April 30, 2020 11:13

Use partial in set_ticklabels instead of an argument in FuncFormatter.

6bfbb6c

Update selectively instead of using _update_ticks.

b217c3f

anntzer reviewed Apr 30, 2020

View reviewed changes

anntzer approved these changes Apr 30, 2020

View reviewed changes

Slight simplification.

6c71b67

efiring added this to the v3.3.0 milestone May 4, 2020

QuLogic reviewed May 4, 2020

View reviewed changes

Require a match between fixed ticks and ticklabels.

dbe6f35

jklymak reviewed May 4, 2020

View reviewed changes

Fix broken example, add to api_changes, tweak error message.

6291430

efiring added 2 commits May 4, 2020 13:48

Fix example: second try.

6339f65

Typo in api_changes entry

da16bce

Improvements to docstring and to API docs.

3eb471d

jklymak approved these changes May 6, 2020

View reviewed changes

tacaswell merged commit 8d33af7 into matplotlib:master May 11, 2020

This was referenced Jun 20, 2020

MNT: fix some failures with matplotlib=3.3.0rc1 mwaskom/seaborn#2144

Merged

ax.set_xticklabels([]) for categorical plots is broken in 3.3.0rc1 #17736

Closed

jklymak mentioned this pull request Jul 23, 2020

Matplotlib 3.3 compatibility fixups pandas-dev/pandas#35393

Merged

wtgee mentioned this pull request Aug 25, 2020

plot_sky error astropy/astroplan#468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep explicit ticklabels in sync with ticks from FixedLocator #17266

Keep explicit ticklabels in sync with ticks from FixedLocator #17266

efiring commented Apr 29, 2020

QuLogic commented Apr 30, 2020

anntzer Apr 30, 2020

anntzer Apr 30, 2020

anntzer Apr 30, 2020

efiring commented Apr 30, 2020

anntzer Apr 30, 2020

efiring Apr 30, 2020

anntzer left a comment

QuLogic May 4, 2020 •

edited

Loading

efiring May 4, 2020

QuLogic May 4, 2020

efiring May 4, 2020

QuLogic May 5, 2020

anntzer May 6, 2020

jklymak commented May 4, 2020

jklymak May 4, 2020

efiring May 4, 2020

jklymak May 5, 2020

efiring commented May 4, 2020

efiring commented May 5, 2020

efiring commented May 5, 2020

jklymak commented May 6, 2020

Keep explicit ticklabels in sync with ticks from FixedLocator #17266

Keep explicit ticklabels in sync with ticks from FixedLocator #17266

Conversation

efiring commented Apr 29, 2020

PR Summary

PR Checklist

QuLogic commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiring commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer left a comment

Choose a reason for hiding this comment

QuLogic May 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak commented May 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiring commented May 4, 2020

efiring commented May 5, 2020

efiring commented May 5, 2020

jklymak commented May 6, 2020

QuLogic May 4, 2020 •

edited

Loading