NotificationsYou must be signed in to change notification settings
Fork8.1k
Star22.1k

[MNT]: Cleanup ticklabel_format (style=)#26801

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

timhoffm merged 8 commits intomatplotlib:mainfrompedrompecanha:cleanup-ticklabel

Sep 20, 2023

Merged

[MNT]: Cleanup ticklabel_format (style=)#26801

timhoffm merged 8 commits intomatplotlib:mainfrompedrompecanha:cleanup-ticklabel

Sep 20, 2023

Conversation

Copy link

Contributor

pedrompecanha commentedSep 17, 2023•
edited
Loading

PR summary

Fixes#25974.
-->

PR checklist

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

pedrompecanha added2 commits

September 17, 2023 13:28

Changing the default value to None, adding docstring documenting and …

c67d1c3

…fixing extra logic.

Update _base.py

b660799

Fixing if condition.

github-actionsbot reviewed

Sep 17, 2023

View reviewed changes

Copy link

github-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

rcomer reviewed

Sep 18, 2023

View reviewed changes

Copy link

Member

rcomer left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this@pedrompecanha! Just a couple of comments on the implementation...

This line will break ifstyle isNone, so perhaps should be moved into theelse of your new if loop below.

matplotlib/lib/matplotlib/axes/_base.py

Line 3277 in6fa5045

style=style.lower()

lib/matplotlib/axes/_base.py Outdated

		is_sci_style=_api.check_getitem(STYLES,style=style)
		STYLES= {'sci':True,'scientific':True,'plain':False}
		ifstyleisNone:
		is_sci_style=False

Copy link

Member

rcomerSep 18, 2023

Choose a reason for hiding this comment

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

Suggested change

	is_sci_style=False
	is_sci_style=None

None here should mean that the style is not changed from what it previously was. Settingis_sci_style also toNone achieves that.

Copy link

ContributorAuthor

pedrompecanhaSep 18, 2023

Choose a reason for hiding this comment

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

I'm changing it so that everything is in the dictionary. I believe it solves it more effectively. About setting it to None or False, I thought this variable should be either True or False but, after reading the logic after it, I've seen it can be None, indeed.

Copy link

Member

rcomer commentedSep 18, 2023•
edited
Loading

The typing stub file will also need updating to match the new signature

matplotlib/lib/matplotlib/axes/_base.pyi

Lines 282 to 291 in6fa5045

	defticklabel_format(
	self,
	*,
	axis:Literal["both","x","y"]= ...,
	style:Literal["","sci","scientific","plain"]= ...,
	scilimits:tuple[int,int]\|None= ...,
	useOffset:bool\|float\|None= ...,
	useLocale:bool\|None= ...,
	useMathText:bool\|None= ...
	)->None: ...

oscargus reviewed

Sep 18, 2023

View reviewed changes

lib/matplotlib/axes/_base.py Outdated

		)fromerr
		STYLES= {'sci':True,'scientific':True,'plain':False,'':None}
		is_sci_style=_api.check_getitem(STYLES,style=style)
		STYLES= {'sci':True,'scientific':True,'plain':False}

Copy link

Member

oscargusSep 18, 2023•
edited
Loading

Choose a reason for hiding this comment

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

An option here (and also in case someone has setstyle='' explicitly), is to do:

Suggested change

	STYLES= {'sci':True,'scientific':True,'plain':False}
	STYLES= {'sci':True,'scientific':True,'plain':False,'':None,None:None}
	is_sci_style=_api.check_getitem(STYLES,style=style)

Copy link

Member

rcomerSep 18, 2023

Choose a reason for hiding this comment

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

in case someone has setstyle='' explicitly

That's a good point. I suggest to also add a comment that'' is included for backwards-compatibility, as I do not think it would be obvious to future readers of the code.

Copy link

ContributorAuthor

pedrompecanhaSep 18, 2023

Choose a reason for hiding this comment

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

I added the comment and added the empty string into the dictionary. Actually, I was playing tennis and thought about how my changes weren't backwards compatible. Nice to see we are in the same page :)

rcomer mentioned this pull request

Sep 18, 2023

[MNT]: Cleanup ticklabel_format(..., style=)#25991

Closed

3 tasks

Added backwards-compatibility comment, changing to a solo STYLES dict…

2096ae7

…ionary and added stub parameter alternative.

rcomer reviewed

Sep 18, 2023

View reviewed changes

lib/matplotlib/axes/_base.py Outdated

		Whether to use scientific notation.
		The formatter default is to use scientific notation.
		Sci is equivalent to scientific.
		The '' option is included solely for backwards-compatibility.

Copy link

Member

rcomerSep 18, 2023

Choose a reason for hiding this comment

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

Apologies, I think I was unclear. I meant to add a code comment where the dictionary is defined, so that future developers know that it’s there for a reason. I don’t think it’s needed in the docstring.

Copy link

ContributorAuthor

pedrompecanhaSep 18, 2023

Choose a reason for hiding this comment

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

Done in the latest commit!

Changing comments from the docstring area to inline comments.

c6fdf59

rcomer reviewed

Sep 19, 2023

View reviewed changes

Copy link

Member

rcomer left a comment

Choose a reason for hiding this comment

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

The last test failure comes withinstructions (which I admit are new to me!)

lib/matplotlib/axes/_base.py Outdated

		)fromerr
		STYLES= {'sci':True,'scientific':True,'plain':False,'':None}
		STYLES= {'sci':True,'scientific':True,'plain':False,'':None,None:None}
		# 'sci' is equivalent to 'scientific'.

Copy link

Member

rcomerSep 19, 2023

Choose a reason for hiding this comment

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

I think the fact that 'sci' and ‘scientific' are equivalentshould be in the docstring, as this is useful information for the user.

Copy link

ContributorAuthor

pedrompecanhaSep 19, 2023

Choose a reason for hiding this comment

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

Just put them back there! As for the pyplot issue, I attempted following the instructions and it told me, in my system, that the file got reformatted. Let's see how it goes!

Copy link

Member

rcomerSep 19, 2023

Choose a reason for hiding this comment

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

It seems the pyplot change has not appeared here on GitHub...

Copy link

ContributorAuthor

pedrompecanhaSep 19, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Yeah! I tried running the command again, but the file remained unchanged. I tried to change the file by hand, although this is not encouraged, and the commit went through on my fork, but it won't open here on Github. Just goes straight to a 404 page. Weird.

Here is the final change I made:pedrompecanha@3cf0234

Copy link

ContributorAuthor

pedrompecanhaSep 19, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I believe it has gone through now. Must have been a GitHub instability. The tests seem to be running well, although I'm kind of scared of having to change this file by hand. Thoughts?

Copy link

Member

rcomerSep 19, 2023

Choose a reason for hiding this comment

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

Your manual change matches what I get by running the script, so I think it's OK.

pedrompecanha added3 commits

September 19, 2023 09:30

Reformatting pyplot.py and putting the sci equivalency in the docstri…

4c3355d

…ng area.

Attempting to fix pyplot.py by hand, although this is not encouraged.

558b2d8

Adding a comma to pyplot.py.

3cf0234

rcomer reviewed

Sep 19, 2023

View reviewed changes

Copy link

Member

rcomer left a comment

Choose a reason for hiding this comment

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

Thanks@pedrompecanha, I think this is in good shape now. I just have a tiny suggestion on the docstring.

lib/matplotlib/axes/_base.py OutdatedShow resolvedHide resolved

Editing the docstring, making it more consistent to the other paramet…

6bdc99f

…ers that default to None.Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>

rcomer approved these changes

Sep 19, 2023

View reviewed changes

Copy link

ContributorAuthor

pedrompecanha commentedSep 19, 2023

@rcomer,@timhoffm and@oscargus, thank you for the support and patience during this process! I'd like to ask that, if you find any similar cleanup or refactoring tasks, if you could please tag me, as I'll be really active in the next 1-2 months in here. Of course, if that is allowed by the rules of the project. I'll try my best to solve the issues. Again, thank you!