NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

WARN: more direct warning ticklabels#26401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ksunden merged 1 commit intomatplotlib:mainfromjklymak:warn-better-ticklabels

Aug 3, 2023

Merged

WARN: more direct warning ticklabels#26401

ksunden merged 1 commit intomatplotlib:mainfromjklymak:warn-better-ticklabels

Aug 3, 2023

Conversation

Copy link

Member

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))

importnumpyasnpfrommatplotlibimportcmimportmatplotlib.pyplotaspltforadjustin [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

"closes #0000" is in the body of the PR description tolink the related issue
new and changed code istested
Plotting related features are demonstrated in anexample
New Features andAPI Changes are noted with adirective and release note
Documentation complies withgeneral anddocstring guidelines

Copy link

MemberAuthor

So the failed tests bring up a use case forset_xticklabels that I don't think our discouragement of the function properly took into account, and that isax.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 ofset_xticklabels would beax.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 force-pushed thewarn-better-ticklabels branch 2 times, most recently from314785e tob0c11c4Compare

July 30, 2023 05:09

Copy link

MemberAuthor

jklymak commentedJul 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)

Copy link

Member

rcomer commentedJul 30, 2023

I have opened#26414 for thoseflake8 failures.

jklymak marked this pull request as ready for review

July 30, 2023 15:05

jklymak force-pushed thewarn-better-ticklabels branch fromb0c11c4 todc667deCompare

July 30, 2023 15:05

timhoffm reviewed

Jul 30, 2023

View reviewed changes

lib/matplotlib/axis.py

		if isinstance(locator, mticker.FixedLocator):
		if not labels:
		# eg labels=[]:
		formatter = mticker.NullFormatter()

Copy link

Member

timhoffmJul 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

Member

timhoffmJul 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

MemberAuthor

jklymakJul 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

Member

rcomerJul 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

MemberAuthor

jklymakJul 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.

Copy link

Member

timhoffm commentedJul 31, 2023

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

timhoffm reviewed

Jul 31, 2023

View reviewed changes

lib/matplotlib/axis.py OutdatedShow resolvedHide resolved

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

Member

timhoffmJul 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 aroundset_..._formatter

Copy link

MemberAuthor

jklymakAug 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.

lib/matplotlib/axis.py

		if isinstance(locator, mticker.FixedLocator):
		if not labels:
		# eg labels=[]:
		formatter = mticker.NullFormatter()

Copy link

Member

timhoffmJul 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 force-pushed thewarn-better-ticklabels branch fromdc667de toc472067Compare

August 1, 2023 21:07

tacaswell added this to thev3.8.0 milestone

Aug 2, 2023

tacaswell approved these changes

Aug 2, 2023

View reviewed changes

Copy link

Member

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.

Copy link

MemberAuthor

jklymak commentedAug 2, 2023•
edited
Loading

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

Should be fine now

jklymak force-pushed thewarn-better-ticklabels branch fromc472067 to871f0e4Compare

August 3, 2023 15:22

WARN: fix warning for set_ticklabels

dd2e6f8

jklymak force-pushed thewarn-better-ticklabels branch from871f0e4 todd2e6f8Compare

August 3, 2023 16:21

ksunden approved these changes

Aug 3, 2023

View reviewed changes

ksunden merged commit440f761 intomatplotlib:main

Aug 3, 2023

Labels

None yet

5 participants

Movatterモバイル変換

Uh oh!

WARN: more direct warning ticklabels#26401

WARN: more direct warning ticklabels#26401

Uh oh!

Conversation

jklymak commentedJul 27, 2023

PR summary

PR checklist

Uh oh!

jklymak commentedJul 29, 2023

Uh oh!

jklymak commentedJul 30, 2023

Uh oh!

rcomer commentedJul 30, 2023

Uh oh!

timhoffmJul 30, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timhoffmJul 31, 2023

Choose a reason for hiding this comment

Uh oh!

jklymakJul 31, 2023

Choose a reason for hiding this comment

Uh oh!

rcomerJul 31, 2023

Choose a reason for hiding this comment

Uh oh!

jklymakJul 31, 2023

Choose a reason for hiding this comment

Uh oh!

timhoffm commentedJul 31, 2023

Uh oh!

Uh oh!

timhoffmJul 31, 2023

Choose a reason for hiding this comment

Uh oh!

jklymakAug 3, 2023

Choose a reason for hiding this comment

Uh oh!

timhoffmJul 31, 2023

Choose a reason for hiding this comment

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commentedAug 2, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

timhoffmJul 30, 2023•
edited
Loading

jklymak commentedAug 2, 2023•
edited
Loading