Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Automatically create tick formatters for str and callable inputs.#16715

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

Merged
story645 merged 1 commit intomatplotlib:masterfromtoddrjen:easyformatter
May 3, 2020

Conversation

toddrjen
Copy link
Contributor

PR Summary

This allowsmatplotlib.axis.Axis.set_major_formatter andmatplotlib.axis.Axis.set_minor_formatter to accept astr or function (really anycallable) in addition to amatplotlib.ticker.Formatter. This will create and use amatplotlib.ticker.StrMethodFormatter andmatplotlib.ticker.FuncFormatter, respectively. This significantly reduces the barrier to using custom formatters in simple cases.

This was previouslydiscussed on the matplotlib-devel mailing list.

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 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

stefraynaud reacted with thumbs up emoji
@toddrjentoddrjenforce-pushed theeasyformatter branch 3 times, most recently from3744780 to135fb1cCompareMarch 10, 2020 01:34
@tacaswelltacaswell added this to thev3.3.0 milestoneMar 10, 2020
@tacaswell
Copy link
Member

Thanks@toddrjen !

To try and make sure feature PRs don't fall through the cracks, we starting to assign champions who are responsible for making sure the review process goes through,@story645 has agreed to champion this PR.

@toddrjen
Copy link
ContributorAuthor

@tacaswell@story645 Great, thank you!

I don't know why the test coverage has dropped, I added new tests that, as far as I can tell, cover the new code paths.

Assuming this is a good approach, the big outstanding issue in my mind is whether this should be used more in the documentation. There are several examples using the old-styleFormatStrFormatter that could be switched to using this approach.

@toddrjentoddrjenforce-pushed theeasyformatter branch 2 times, most recently from51d0d1d toe07c5dfCompareMarch 10, 2020 02:58
@story645
Copy link
Member

@toddrjen can you please provide some examples here cause I'm struggling a bit in finding 'em in the PR

@toddrjen
Copy link
ContributorAuthor

@story645 One example for thestr approach used in the documentation is:

axs[5].xaxis.set_major_formatter('{x} km')

Which would replace:

frommatplotlibimporttickeraxs[5].xaxis.set_major_formatter(ticker.StrMethodFormatter('{x} km'))

One for the function approach used in the documentation:

axs[3].xaxis.set_major_formatter(lambdax,pos:str(x-5))

Which would replace:

frommatplotlibimporttickeraxs[3].xaxis.set_major_formatter(ticker.FuncFormatter(lambdax,pos:str(x-5)))

@jklymak
Copy link
Member

In the pr it’s not clear that you need a pos argument to your function or why.

@toddrjen
Copy link
ContributorAuthor

@jklymak Where, exactly, do you think this should be mentioned? Can you comment on the specific lines or blocks where you think this should be changed? In most cases I am just using or following up on the existing description for theFuncFormatter.

@jklymak
Copy link
Member

@jklymak Where, exactly, do you think this should be mentioned? Can you comment on the specific lines or blocks where you think this should be changed? In most cases I am just using or following up on the existing description for theFuncFormatter.

It needs to be documented somewhere, otherwise how are people to know what a valid callable is?

@toddrjen
Copy link
ContributorAuthor

@jklymak Please see the updated version. I have added more explicit documentation.

@toddrjentoddrjenforce-pushed theeasyformatter branch 3 times, most recently from79b548b to43b395cCompareMarch 17, 2020 03:15
@toddrjen
Copy link
ContributorAuthor

I have added a new commit where I modified a bunch of existing examples and tutorials, and the previous documentation, to make better use of the new syntax. These changes may be more controversial so I added them to a separate commit so they would be easier to roll back.

Please take a look and see what you think.

I don't know why the tests are failing. It doesn't appear to be related to my commit but I am not sure.

@toddrjen
Copy link
ContributorAuthor

@story645 All tests are passing now. Is there anything else I need to do or is this ready?

@story645
Copy link
Member

story645 commentedApr 1, 2020
edited
Loading

Sorry I still owe you feedback on this, just wanted to note that I'm +1 on this idea since learning that spotify is basically using this sort of thing as theirmotivating example for writing a new charting library.

Copy link
Member

@story645story645 left a comment

Choose a reason for hiding this comment

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

a lot of especially the language changes are more suggestions than must changes, but I really want the test cases to be broken up

@toddrjentoddrjenforce-pushed theeasyformatter branch 2 times, most recently frombd151ee to642928dCompareApril 6, 2020 18:12
@toddrjen
Copy link
ContributorAuthor

@story645 All the discussed changes should be implemented. Please take a look. I am still waiting on feedback on a few issues.

@story645
Copy link
Member

Thanks for your patience, been swamped with Passover.@jklymak and@anntzer have your concerns been addressed?

@anntzer
Copy link
Contributor

As stated above I'm basically fine with the PR, but don't like one very specific point (direct support for strings) so don't particularly want to approve it with that feature.

Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

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

This seems good, and I think will help 99% of folks who don't want to learn the arcana of Formatters. Do we want to replaceevery instance ofStrMethodFormatter andFuncFormatter by this idiom? There is some advantage to pointing out the ticker module and the existence of the suite of Formatters.

@toddrjen
Copy link
ContributorAuthor

@jklymak

Do we want to replaceevery instance ofStrMethodFormatter andFuncFormatter by this idiom? There is some advantage to pointing out the ticker module and the existence of the suite of Formatters.

I didn't replace every instance.examples/ticks_and_spines/tick-formatters.py still explains how to use the existing method.

jklymak reacted with thumbs up emoji

@story645
Copy link
Member

Hi Todd, thanks for hanging in there! We're aiming to get this into 3.3 (rc out in a week or so).

Giving it some more thought, keeping the current formatter signature seems fine as it's consistent with the docs and what folks will find on stackoverflows. Can revisit in a later PR.

Do you feel it's ready to merge or do you have any additions to make?

@story645
Copy link
Member

Also now thinking the arguments fix should actually be on FuncFormatter since it's not clear why it needs twohttps://matplotlib.org/_modules/matplotlib/ticker.html#FuncFormatter

@toddrjen
Copy link
ContributorAuthor

@story645 If all the tests pass I think it is ready to merge.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@story645story645story645 approved these changes

@jklymakjklymakjklymak approved these changes

Assignees

@story645story645

Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

6 participants
@toddrjen@tacaswell@story645@jklymak@anntzer@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp