BUG: Prevent Tick params calls from overwriting visibility without being told to #12839

ksunden · 2018-11-19T18:01:58Z

PR Summary

Prevents a regression bisected to #10088, which caused subsequent calls to tick_params to overwrite the visibility of tick labels (et al.) when those calls did not set visibility themselves.

Just pops a subset of the kwargs out of the dictionary and replaces them after the call to _apply_params is made, so that they are there for other parts of the codebase (including tests).

The test included here passes on current release (v3.0.2), but fails on current master (
936855c).

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

42B · 2018-11-19T18:26:39Z

lib/matplotlib/axis.py

        for d in dicts:
            if reset:
                d.clear()
+            blacklist_dicts.append(
+                    {k: d.pop(k) for k in blacklist if k in d.keys()}


First, either this line seems to be indented too far or the line above isn't indented enough? Second, I'm curious about if there is a better way to write this without using .pop and iterating through d.keys() for every single lookup?

I'll note that the iteration is only over the keys in the blacklist (total of 5 items), k in d.keys() is an O(1) check since keys view objects are set-like and hashable.

The second iteration could actually be rolled into this conditional, to prevent things from being popped then added back with updates, rather than popping indiscriminately and then iterating over the kwtrans keys.

jklymak · 2018-11-29T17:05:28Z

lib/matplotlib/tests/test_axes.py

+    fig, ax = plt.subplots()
+    plt.plot([0, 1, 2], [0, -1, 4])
+    plt.setp(ax.get_yticklabels(), visible=False)
+    ax.tick_params(axis="y", which="both", length=0)


I think this test could be easily done without an image comparison by just checking the visibility of the ticks?

The tricky bit is that one of the common ways in tests of checking wheter the ticks are visible was lying to you prior to this PR, (though if you went to the individual ticks, it would be correct) As such, I was looking to make sure it translated to the image generated.

You can instead save to svg and use some long-ish strings as labels, and check that the labels don't appear as substrings in the svg file.

jklymak · 2018-11-29T17:14:34Z

lib/matplotlib/axis.py

        for d in dicts:
            if reset:
                d.clear()
+            blacklist_dicts.append(


I find this pretty opaque, and don’t really understand why these kwargs are different than the other kwargs (other than that they control visibility). This could really use a comment explaining what it does and why. As it stands, I’m not clear at all that this is the proper solution, and maybe there is something more fundamental that is at fault.

anntzer · 2019-02-07T09:45:48Z

So the bug comes from the fact that plt.setp(... visible=False) set the visibility by calling tick.set_visible(False), which used to override tick_params() which would set reset label1On to True; but after #10088 label1On maps to set_visible and thus the call to tick_params undoes the call to set_visible(False).

I think a correct fix would be somewhere along the lines of

diff --git i/lib/matplotlib/axis.py w/lib/matplotlib/axis.py
index 127c9c5b0..094cd53fd 100644
--- i/lib/matplotlib/axis.py
+++ w/lib/matplotlib/axis.py
@@ -856,26 +856,16 @@ class Axis(martist.Artist):
         For documentation of keyword arguments, see
         :meth:`matplotlib.axes.Axes.tick_params`.
         """
-        dicts = []
-        if which == 'major' or which == 'both':
-            dicts.append(self._major_tick_kw)
-        if which == 'minor' or which == 'both':
-            dicts.append(self._minor_tick_kw)
         kwtrans = self._translate_tick_kw(kw)
-        for d in dicts:
-            if reset:
-                d.clear()
-            d.update(kwtrans)
-
         if reset:
             self.reset_ticks()
         else:
             if which == 'major' or which == 'both':
                 for tick in self.majorTicks:
-                    tick._apply_params(**self._major_tick_kw)
+                    tick._apply_params(**kwtrans)
             if which == 'minor' or which == 'both':
                 for tick in self.minorTicks:
-                    tick._apply_params(**self._minor_tick_kw)
+                    tick._apply_params(**kwtrans)
             if 'labelcolor' in kwtrans:
                 self.offsetText.set_color(kwtrans['labelcolor'])
         self.stale = True

plus some wrangling with the rest of the codebase to keep track of label1On-ness.

Release critical as a regression.

ksunden · 2019-02-07T16:54:34Z

If I recall correctly, I had tried something like the simpler diff you presented here, but got several new test failures as a result. It may very well be easy to track those down in a cleaner way, though my inital efforts focused on fixing the problem fairily locally.

anntzer · 2019-02-11T10:19:43Z

I think a simpler approach may be possible but this looks generally ok as a stopgap if nothing better is proposed before 3.1.

anntzer · 2019-02-11T10:19:55Z

lib/matplotlib/axis.py

+            blacklist_dicts.append(
+                {
+                    k: d.pop(k) for k in blacklist
+                    if k in d.keys() and k not in kwtrans.keys()


anntzer

Strongly suggest not adding a baseline image per https://github.com/matplotlib/matplotlib/pull/12839/files#r255441630.

tacaswell · 2019-02-18T21:29:35Z

I am concerned about this fix. The underlying issue here is that the tick system has an internal set of defaults that it uses to create new ticks, but by using plt.setp you are reaching in to change the state of the ticks without propagating that back the default dictionary. This skip-list works for visibility, but will not work for color (or any other property).
Doing

ax.tick_params(axis="y", which="both", label1On=False)
ax.tick_params(axis="y", which="both", length=0)

works as expected.

I am 👎 on merging this. The correct fix is to track on the tick objects what state has been touch independently by the user and then not update that property.

I am also concerned that this will not work in the case where we generate more tick objects later so you will end up with mixed state.

tacaswell

I do not think this is the correct fix to the problem (which is that we do not track if users have mutated objects that the axis thinks it is managing).

tacaswell · 2019-02-18T21:31:27Z

It might be possible to use the get methods to check what state does not match the (previous) defaults and then skip applying based on that?

ksunden · 2019-02-19T03:40:32Z

To be clear, this is just taking out arguments only in the case that they were not asked for explicitly and simply not trying to change it. If the user is modifying the state using this function, those changes are propegated.

That said, I make no illusions, this is a hack, and there are certainly cleaner solutions, including changing my own library code such that the labelOn, etc. get set to match.

The point about mixed state is a good one, however, if users intentionally got a mixed state (I would tend to avoid this myself, but still) The code without this patch will override that mixed state, even when the arguments that are written have nothing to do with visibility.

The way this is implemented preserves mixed state if it exists, but makes it easy to get back to a consistent state.

tacaswell · 2019-02-23T17:43:46Z

Sorry, being thrashy about milestones / tags... Still thinking about this.

I have now pushed a commit to this branch.

tacaswell · 2019-02-23T18:00:37Z

I applied half of the diff suggested in the comments by @anntzer (we still want to update the default settings for new ticks) but only apply the passed-in kwargs to the existing ticks.

If I recall correctly, I had tried something like the simpler diff you presented here, but got several new test failures as a resul

@ksunden what tests failed for you locally with this change?

Went back and forth a bit on trying to understand what the intent of this method is (as it could be argued that the correct behavior is to re-apply the standard style to all of the ticks), but the presence of the reset kwargs suggests otherwise.

ksunden · 2019-02-23T22:20:20Z

I am not getting any local test failures now, I recall trying this back in november, but don't recall why I ultimately moved on from it. All looks good on my end now.

anntzer

Please try to not include a baseline image per https://github.com/matplotlib/matplotlib/pull/12839/files#r255441630. (I'm not 100% sure this will work, but is at least worth a try.)

Saving 15Kb on a file is not worth the complexity of checking the text of an svg (discussed on phone call)

dopplershift

I'm ok with adding another baseline image. It's only 15kb, and the alternative would actually be something out of line from the rest of the test suite and require more effort to understand what's going on.

…rwriting visibility without being told to

lumberbot-app · 2019-02-25T20:10:40Z

Something went wrong ... Please have a look at my logs.

…839-on-v3.1.x Backport PR #12839 on branch v3.1.x (BUG: Prevent Tick params calls from overwriting visibility without being told to)

After matplotlib/matplotlib#12839 axis ticks showed up along all the edges of the plot panel and theming of tick labels was finicky. The solution is we have to be careful in how we toggle the visibility of these elements. If not careful, merely setting the tick/tick labels could switch on the visibility. This could interfere with theme. closes #278

ksunden added 3 commits November 19, 2018 10:22

BUG: Prevent overwriting of tick visiblity

e4dafa2

TST: add test to prevent regression of tick visibility behavior

ff2759a

MAINT: flake8 fixes

0a82d82

42B reviewed Nov 19, 2018

View reviewed changes

ksunden added 2 commits November 19, 2018 14:30

MAINT: more flake8/whitespace fixes

3cc6b00

REV: Remove second iteration by not placing in blacklist dict

b7a4db0

jklymak reviewed Nov 29, 2018

View reviewed changes

anntzer added this to the v3.1.0 milestone Feb 7, 2019

anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 7, 2019

anntzer reviewed Feb 11, 2019

View reviewed changes

anntzer requested changes Feb 18, 2019

View reviewed changes

tacaswell previously requested changes Feb 18, 2019

View reviewed changes

tacaswell removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 23, 2019

tacaswell modified the milestones: v3.1.0, v3.2.0 Feb 23, 2019

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 23, 2019

tacaswell modified the milestones: v3.2.0, v3.1.0 Feb 23, 2019

FIX: only update the kwargs asked for in tick_params

d501de7

tacaswell requested a review from anntzer February 23, 2019 17:55

tacaswell approved these changes Feb 24, 2019

View reviewed changes

anntzer previously requested changes Feb 24, 2019

View reviewed changes

dopplershift approved these changes Feb 25, 2019

View reviewed changes

dopplershift merged commit eb92f62 into matplotlib:master Feb 25, 2019

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Feb 25, 2019

Backport PR matplotlib#12839: BUG: Prevent Tick params calls from ove…

8299d68

…rwriting visibility without being told to

meeseeksmachine mentioned this pull request Feb 25, 2019

Backport PR #12839 on branch v3.1.x (BUG: Prevent Tick params calls from overwriting visibility without being told to) #13518

Merged

jklymak added a commit that referenced this pull request Feb 25, 2019

Merge pull request #13518 from meeseeksmachine/auto-backport-of-pr-12…

4873c3c

…839-on-v3.1.x Backport PR #12839 on branch v3.1.x (BUG: Prevent Tick params calls from overwriting visibility without being told to)

ksunden deleted the tick_params branch February 26, 2019 00:05

ksunden restored the tick_params branch March 29, 2019 16:22

has2k1 mentioned this pull request May 29, 2019

extra axis ticks being added has2k1/plotnine#278

Closed

ksunden deleted the tick_params branch December 27, 2022 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Prevent Tick params calls from overwriting visibility without being told to #12839

BUG: Prevent Tick params calls from overwriting visibility without being told to #12839

ksunden commented Nov 19, 2018

42B Nov 19, 2018 •

edited

Loading

ksunden Nov 19, 2018 •

edited

Loading

jklymak Nov 29, 2018

ksunden Feb 7, 2019

anntzer Feb 11, 2019

jklymak Nov 29, 2018

anntzer commented Feb 7, 2019

ksunden commented Feb 7, 2019

anntzer commented Feb 11, 2019

anntzer Feb 11, 2019

anntzer left a comment

tacaswell commented Feb 18, 2019

tacaswell left a comment

tacaswell commented Feb 18, 2019

ksunden commented Feb 19, 2019

tacaswell commented Feb 23, 2019

tacaswell commented Feb 23, 2019

ksunden commented Feb 23, 2019

anntzer left a comment

dopplershift left a comment

lumberbot-app bot commented Feb 25, 2019

BUG: Prevent Tick params calls from overwriting visibility without being told to #12839

BUG: Prevent Tick params calls from overwriting visibility without being told to #12839

Conversation

ksunden commented Nov 19, 2018

PR Summary

PR Checklist

42B Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

ksunden Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

jklymak Nov 29, 2018

Choose a reason for hiding this comment

ksunden Feb 7, 2019

Choose a reason for hiding this comment

anntzer Feb 11, 2019

Choose a reason for hiding this comment

jklymak Nov 29, 2018

Choose a reason for hiding this comment

anntzer commented Feb 7, 2019

ksunden commented Feb 7, 2019

anntzer commented Feb 11, 2019

anntzer Feb 11, 2019

Choose a reason for hiding this comment

anntzer left a comment

Choose a reason for hiding this comment

tacaswell commented Feb 18, 2019

tacaswell left a comment

Choose a reason for hiding this comment

tacaswell commented Feb 18, 2019

ksunden commented Feb 19, 2019

tacaswell commented Feb 23, 2019

tacaswell commented Feb 23, 2019

ksunden commented Feb 23, 2019

anntzer left a comment

Choose a reason for hiding this comment

dopplershift left a comment

Choose a reason for hiding this comment

lumberbot-app bot commented Feb 25, 2019

42B Nov 19, 2018 •

edited

Loading

ksunden Nov 19, 2018 •

edited

Loading