Skip to content

WARN: more direct warning ticklabels#26401

Merged
ksunden merged 1 commit intomatplotlib:mainfrom
jklymak:warn-better-ticklabels
Aug 3, 2023
Merged

WARN: more direct warning ticklabels#26401
ksunden merged 1 commit intomatplotlib:mainfrom
jklymak:warn-better-ticklabels

Conversation

@jklymak
Copy link
Copy Markdown
Member

@jklymak jklymak commented Jul 27, 2023

PR summary

Closes #26398

This now warns with

/Users/jklymak/matplotlib/testit.py:11: UserWarning: set_ticklabels should only be used with set_ticks or a FixedLocator.
  ax.set_yticklabels(map(str, ticks))

import numpy as np
from matplotlib import cm
import matplotlib.pyplot as plt

for adjust in [True]:
    fig, ax = plt.subplots(figsize=(5, 3))
    data = [33.4, 33.5, 33.6]
    ax.plot(data, data)
    ticks = [32.8, 33., 33.2, 33.4, 33.6, 33.8]
    # ax.set_yticks(ticks)
    ax.set_yticklabels(map(str, ticks))

    fig.subplots_adjust(top=0.96)
  • Needs tests

PR checklist

@jklymak
Copy link
Copy Markdown
Member Author

jklymak commented Jul 29, 2023

So the failed tests bring up a use case for set_xticklabels that I don't think our discouragement of the function properly took into account, and that is ax.set_xticklabels([]) to stop labelling the ticks, but to keep the actual ticks. The only user-accessible way to do this if we were to get rid of set_xticklabels would be ax.xaxis.set_major_formatter(ticker.NullFormatter()) which I don't think is very user friendly.

This PR checks for an empty list for tick labels, and short circuits to NullFormatter. However, we should probably decide how we want to do this properly. I believe we did this #20047 (ping @timhoffm) and perhaps needs a bit of a rethink.

@jklymak jklymak force-pushed the warn-better-ticklabels branch 2 times, most recently from 314785e to b0c11c4 Compare July 30, 2023 05:09
@jklymak
Copy link
Copy Markdown
Member Author

jklymak commented Jul 30, 2023

Flake8 is failing for E721 on a bunch of code I didn't change. I guess a new version of flake8 (doesn't fail locally)

@rcomer
Copy link
Copy Markdown
Member

rcomer commented Jul 30, 2023

I have opened #26414 for those flake8 failures.

@jklymak jklymak marked this pull request as ready for review July 30, 2023 15:05
@jklymak jklymak force-pushed the warn-better-ticklabels branch from b0c11c4 to dc667de Compare July 30, 2023 15:05
timhoffm
timhoffm reviewed Jul 30, 2023
View reviewed changes
Comment thread lib/matplotlib/axis.py
if isinstance(locator, mticker.FixedLocator):
if not labels:
# eg labels=[]:
formatter = mticker.NullFormatter()
Copy link
Copy Markdown
Member

@timhoffm timhoffm Jul 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This does not have an effect if you return on the next line. formatter is only used in lines 2005ff.

Copy link
Copy Markdown
Member

@timhoffm timhoffm Jul 31, 2023

Choose a reason for hiding this comment

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

Obviously, this is untested. You can add a test that there is no warning using

with warnings.catch_warnings():
    warnings.simplefilter("error")

https://docs.pytest.org/en/7.0.x/how-to/capture-warnings.html#additional-use-cases-of-warnings-in-tests

Copy link
Copy Markdown
Member Author

@jklymak jklymak Jul 31, 2023

Choose a reason for hiding this comment

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

Oops thanks I knew there was a reason to not do that.

It is tested, but the old code ends up being used

I'll move to draft as I'm away for a few days

Copy link
Copy Markdown
Member

@rcomer rcomer Jul 31, 2023

Choose a reason for hiding this comment

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

Is the warning filter necessary, since we run with warnings as errors in general? C.f. the test added at #26300.

Copy link
Copy Markdown
Member Author

@jklymak jklymak Jul 31, 2023

Choose a reason for hiding this comment

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

It's not necessary but prevents the same warning appearing with two different syntaxes.

@timhoffm
Copy link
Copy Markdown
Member

timhoffm commented Jul 31, 2023

Since #26414 is in, you can rebase to fix flake8.

timhoffm
timhoffm reviewed Jul 31, 2023
View reviewed changes
Comment thread lib/matplotlib/axis.py Outdated
Comment thread lib/matplotlib/axis.py Outdated
locs = self.get_majorticklocs()
ticks = self.get_major_ticks(len(locs))
with warnings.catch_warnings():
warnings.filterwarnings("ignore", message="FixedFormatter should only ")
Copy link
Copy Markdown
Member

@timhoffm timhoffm Jul 31, 2023

Choose a reason for hiding this comment

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

Let's be more explicit:

Suggested change
warnings.filterwarnings("ignore", message="FixedFormatter should only ")
warnings.filterwarnings(
"ignore",
message="FixedFormatter should only be used together with FixedLocator")

Also, I'm tempted to move this in to be only around set_..._formatter

Copy link
Copy Markdown
Member Author

@jklymak jklymak Aug 3, 2023

Choose a reason for hiding this comment

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

Repeat the filter twice? Doesn't seem necessary, though it does make it more obvious what call is getting the filter.

Comment thread lib/matplotlib/axis.py
if isinstance(locator, mticker.FixedLocator):
if not labels:
# eg labels=[]:
formatter = mticker.NullFormatter()
Copy link
Copy Markdown
Member

@timhoffm timhoffm Jul 31, 2023

Choose a reason for hiding this comment

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

Obviously, this is untested. You can add a test that there is no warning using

with warnings.catch_warnings():
    warnings.simplefilter("error")

https://docs.pytest.org/en/7.0.x/how-to/capture-warnings.html#additional-use-cases-of-warnings-in-tests

@jklymak jklymak force-pushed the warn-better-ticklabels branch from dc667de to c472067 Compare August 1, 2023 21:07
@tacaswell tacaswell added this to the v3.8.0 milestone Aug 2, 2023
tacaswell
tacaswell approved these changes Aug 2, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I agree with Tim's suggestions, but leaving an approval to expedite merging once fixed.

@jklymak
Copy link
Copy Markdown
Member Author

jklymak commented Aug 2, 2023 •
edited
Loading

~Ooops yes I forgot to address those yet. Please let me fix before merging. ~

Should be fine now

@jklymak jklymak force-pushed the warn-better-ticklabels branch from c472067 to 871f0e4 Compare August 3, 2023 15:22
@jklymak jklymak force-pushed the warn-better-ticklabels branch from 871f0e4 to dd2e6f8 Compare August 3, 2023 16:21
@ksunden ksunden merged commit 440f761 into matplotlib:main Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timhoffm timhoffm timhoffm left review comments

@rcomer rcomer rcomer left review comments

@tacaswell tacaswell tacaswell approved these changes

@ksunden ksunden ksunden approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.8.0

Development

Successfully merging this pull request may close these issues.

[Bug]: fig.subplots_adjust and ax.set_yticklabels together can produce unexpected results

5 participants

@jklymak @rcomer @timhoffm @tacaswell @ksunden