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

Add new-style string formatting option and callable option tofmt inAxes.bar_label().#23690

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
jklymak merged 10 commits intomatplotlib:mainfromstefmolin:bar-label-fmt
Sep 7, 2022

Conversation

stefmolin
Copy link
Contributor

@stefmolinstefmolin commentedAug 20, 2022
edited
Loading

PR Summary

Addresses#23689

Enhancement toAxes.bar_label() to make it possible to format values with f-strings.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

@stefmolin
Copy link
ContributorAuthor

Example usage:

importmatplotlib.pyplotaspltfig,ax=plt.subplots()bar_container=ax.barh(['c','b','a'], [1_000,5_000,7_000])ax.bar_label(bar_container,label_type='center',fmt='{:,.0f}')

Screen Shot 2022-08-20 at 6 44 41 PM

@stefmolinstefmolin changed the title[ENH] Add f-string option tofmt inAxes.bar_label().[WIP] Add f-string option tofmt inAxes.bar_label().Aug 20, 2022
@story645
Copy link
Member

story645 commentedAug 21, 2022
edited
Loading

I think this is totally reasonable given we allow fstrings directly and indirectly for most of the other labeling, so I'm +1 on this.

ETA: Also, sorry should have posted this on the issue!

@tacaswell
Copy link
Member

In general we have not tried to guess which version of format strings the user passed. In the test used here"label {:f}" and"{%f}" would both fail the wrong way.

This did come up when we added this in 2019#15602 (comment) and we opted for "old style" at the time.

I do not think we should go down the route of guessing which one the user passed (in addition to the two examples above"a: %d b: {:d}" I think will correctly format under either scheme and we have no way to tell which the user meant (I'm sure there are more realistic examples that have the same problem)). We have not done it elsewhere in the library (this is why we have aFormatStrFormatter (old style) andStrMethodFormatter (new style) classes).

I think our options are

  1. add a flag to control how we interpretfmt
  2. add a second (conflicting) keyword for newsytle format (if both are passed error, if neither is passed current behavior)
  3. accept a callable tofmt so you would doax.bar_label(..., fmt="{:g}".format) (we can reliable tell apart a str and a callable)

I think option 3 is my favorite followed by 2.


My understand is that "fstring" refers to when you dof"xy {code}" which is effectivly a string literal and can not exist un-resolved where as this is about "new style format strings".

@tacaswelltacaswell added this to thev3.7.0 milestoneAug 21, 2022
@story645
Copy link
Member

story645 commentedAug 21, 2022
edited
Loading

I think allowing a callable would be great as that would be consistent with the tick formatters and provides the most labeling flexibility, so cool if that's the easiest path forward!

ETA 2:@stefmolin you should probably hold off on changing code until more people chime in on what direction this should take.

ETA: I do think f-strings should be allowed directly since they're so widely used, but shifting that discussion to#23694 since I don't want that discussion to clobber this one.

@stefmolin
Copy link
ContributorAuthor

stefmolin commentedAug 21, 2022
edited
Loading

@tacaswell - Yes, I was referring to new-style format strings – sorry for any confusion.

Going off the other issue, I'm going to update this to accept callables as well.@story645 - I really like the idea of being able to use the tick formatters here as well.

@stefmolinstefmolin changed the title[WIP] Add f-string option tofmt inAxes.bar_label().[WIP] Add new-style string formatting option and callable option tofmt inAxes.bar_label().Aug 21, 2022
@story645
Copy link
Member

@stefmolin just to be clear, you're 💯% welcome to join in on the formatting discussion where ever it's taking place.

stefmolin reacted with thumbs up emoji

@tacaswell
Copy link
Member

I continue to be very skeptical of trying to guess which format string the user intended.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

The format function needs thorough testing.

@tacaswell
Copy link
Member

It is worth always trying both% and.format and warning (raising) if both work?

To@jklymak 's point about being able to force the users preference, I think "use a lambda" is sufficient for that case.

@timhoffm
Copy link
Member

timhoffm commentedAug 26, 2022
edited
Loading

It is worth always trying both% and.format and warning (raising) if both work?

I originally also thought that way, but now think otherwise

  • "...".format() returns the string unchanged if there is no{} in the string. I.e. for a simple %-style string {} works as well. - You could test that by checking whether the returned string is different, but that is extra effort.
  • by prefering %-style, we have 100% backward compatibility. - That's what we have been doing so far. We only catch the exception that was earlier surfaced to the user, and now try something additionally. In turn, pathological cases like "%d {}" have worked so far and would break if we raise on both working.

Additional note: My only slight concern with prefering %-style is that I would actually prefer {}-style when I had to write that from scratch, because {} is more common nowadays. But I think, that's not important enough to introduce minor backward incompatibilities.

@tacaswell
Copy link
Member

@timhoffm I'm convinced.

@stefmolin What strings were passing the% formatting unexpectedly? That is an awkward construction of re-raising, it would be good to understand exactly why we need it....

timhoffm reacted with hooray emoji

@stefmolin
Copy link
ContributorAuthor

@stefmolin What strings were passing the% formatting unexpectedly? That is an awkward construction of re-raising, it would be good to understand exactly why we need it....

@tacaswell -{:,.0f} was one example where the labels became{:,.0f}. I noticed that anything like{:.2%} didn't need the check – for obvious reasons. Here's a complete example that needed that check to work:

importmatplotlib.pyplotaspltfig,ax=plt.subplots()bar_container=ax.barh(['c','b','a'], [1_000,5_000,7_000])ax.bar_label(bar_container,label_type='center',fmt='{:,.0f}')

Screen Shot 2022-08-26 at 1 25 13 PM

@tacaswell
Copy link
Member

I can reproduce this 🤯 , but I think I have an explanation:

If you put a breakpoint in the formatting function you get to:

fmt='{:,.0f}'here fmt='{:,.0f}' value=1000.0 fmt % value='{:,.0f}'> /home/tcaswell/source/p/matplotlib/matplotlib/lib/matplotlib/cbook/__init__.py(2341)_auto_format_str()-> return fmt % value(Pdb) fmt'{:,.0f}'(Pdb) value1000.0(Pdb) fmt % value'{:,.0f}'(Pdb) fmt % 5*** TypeError: not all arguments converted during string formatting(Pdb) fmt % value'{:,.0f}'(Pdb) fmt % 1000.0*** TypeError: not all arguments converted during string formatting(Pdb) 1000.0 == valueTrue(Pdb) fmt % value'{:,.0f}'(Pdb) fmt % 1000.0*** TypeError: not all arguments converted during string formatting(Pdb) type(value)<class 'numpy.float64'>

so I think the problem here is that if the value is a numpy float the string formatting fails to fail. However it is not that numpy floats fully escape formatting:

In [16]:'s'%np.float64(5)Out[16]:'s'In [17]:'%d'%np.float64(5)Out[17]:'5'

@melissawm have you seen anything like this in numpy before? This seems like a bug, but is rather hard to google...

melissawm reacted with eyes emoji

@tacaswell
Copy link
Member

Ok, thanks pointers from@melissawm and Sebastian, I think I understand what is going on here:

Parsing the documnetation on this very carefully:

If format requires a single argument, values may be a single non-tuple object. 5 Otherwise, values must be a tuple with exactly the number of items specified by the format string, or a single mapping object (for example, a dictionary).

The input can be:

  1. a single value if the format require a single argument
  2. a tuple with exactly the right number of values
  3. a single mapping (which despite the example is really anything with__getitem__)

If we pass a format string with 1% and our value is no a tuple, we hit case 1 and everything is copacetic. However, if we pass a string without any% in it then we try the next two cases. If it is a tuple (and the c code does a type check) then we get case 2. If the value passed is also not a string but does have__getitem__ then we get into case 3 where it is forgiving of some keys in the mapping not being puled out (which makes sense for a use-case where the library provides a whole lot of values and the user can pass in a constructed format string to use the (possibly empty) subset they want.

Becausenp.float64 has__getitem__ defined we end up in case 3 unexpectedly and hence have this issue. By putting the value (which we expect to be treated as a scalar because we told the users to provide us with a string to format a single number) we force case 2.

Other cases that are surprising:

>>>'d'% [1,2,3]'d'>>>'d %(0)d'% [1,2]Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>TypeError:listindicesmustbeintegersorslices,notstr>>>'d %(0)d'% {0:1}Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>KeyError:'0'>>>importnumpyasnp>>>'d %(0)d'%np.float64(5)Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>IndexError:invalidindextoscalarvariable.>>>'d %(0)d'%np.array([1])Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>IndexError:onlyintegers,slices (`:`),ellipsis (`...`),numpy.newaxis (`None`)andintegerorbooleanarraysarevalidindices>>>'d'%np.array([1])'d'>>>'d'% {0:1}'d'

The% formatting code in CPythonhttps://github.com/python/cpython/blob/29f1b0bb1ff73dcc28f0ca7e11794141b6de58c9/Objects/unicodeobject.c#L14262-L14389 (grepping for the error message worked very well here) and the definition ofPyMapping_Checkhttps://github.com/python/cpython/blob/29f1b0bb1ff73dcc28f0ca7e11794141b6de58c9/Objects/abstract.c#L2293-L2300

stefmolin reacted with thumbs up emoji

Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
@stefmolin
Copy link
ContributorAuthor

Thanks for the debugging and explanation@tacaswell - very interesting to understand the why behind that issue.

@stefmolinstefmolin changed the title[WIP] Add new-style string formatting option and callable option tofmt inAxes.bar_label().Add new-style string formatting option and callable option tofmt inAxes.bar_label().Sep 1, 2022
@stefmolin
Copy link
ContributorAuthor

Should I add a what's new entry and/or example for this?

@story645
Copy link
Member

story645 commentedSep 1, 2022
edited
Loading

@stefmolin
Copy link
ContributorAuthor

yes what's new entry, I think update eitherhttps://matplotlib.org/devdocs/gallery/lines_bars_and_markers/bar_label_demo.html#sphx-glr-gallery-lines-bars-and-markers-bar-label-demo-py orhttps://matplotlib.org/devdocs/gallery/lines_bars_and_markers/barchart.html#sphx-glr-gallery-lines-bars-and-markers-barchart-py with the new formatting? (use callables for one group, strings for the other group maybe?)

@story645 – I added a what's new entry, but I'm thinking it might be better to add the examples from the what's new entry to theBar Label Demo. The demo already has multiple examples, but it feels a bit contrived to change the current ones to use the new functionality since there is really no benefit to doing so (there's currently only one example that even usesfmt). What do you think?

For reference, here are the examples I created to make it more apparent why you might use thefmt argument in the new way:

importmatplotlib.pyplotaspltanimal_names= ['Lion','Gazelle','Cheetah']mph_speed= [50,60,75]fig,ax=plt.subplots()bar_container=ax.bar(animal_names,mph_speed)ax.set(ylabel='speed in MPH',title='Running speeds',ylim=(0,80))ax.bar_label(bar_container,fmt=lambdax:'{:.1f} km/h'.format(x*1.61))

Screen Shot 2022-09-03 at 9 20 10 PM

importmatplotlib.pyplotaspltfruit_names= ['Coffee','Salted Caramel','Pistachio']fruit_counts= [4000,2000,7000]fig,ax=plt.subplots()bar_container=ax.bar(fruit_names,fruit_counts)ax.set(ylabel='pints sold',title='Gelato sales by flavor',ylim=(0,8000))ax.bar_label(bar_container,fmt='{:,.0f}')

Screen Shot 2022-09-03 at 9 23 37 PM

story645 reacted with thumbs up emoji

@timhoffm
Copy link
Member

The demo already has multiple examples, but it feels a bit contrived to change the current ones to use the new functionality since there is really no benefit to doing so (there's currently only one example that even uses fmt). What do you think?

IMHO thebar_label demo needs an overhaul anyway. The semanitc content in the existing examples is quite far fetched and doesn't really make sense. I'd like to see more realistic content, like your plots.

For now, it's fine to add your plots to the bar label demo as is.

stefmolin and story645 reacted with thumbs up emoji

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

I still have two minor suggestions, but approve already what is there.

@timhoffm
Copy link
Member

Off-topic:@stefmolin Thanks for working on Matplotlib. I'd like to make you aware of thenew contributor meeting. This is a good place to get to know some of the matplotlib developers, to learn more about the project and of course to ask questions in general. If you are interested, you are very welcome to join. The next date is Tuesday, Sept 6 at 5pm UTC.

story645 and stefmolin reacted with thumbs up emoji

@stefmolin
Copy link
ContributorAuthor

@timhoffm - I've incorporated the suggestions and added the two new examples to the demo. Thanks for letting me know about the new contributor meeting – it's during work for me, but I'll see if I can attend 😄

@story645
Copy link
Member

Holding off on merging only cause I'm not sure if features are fixed when a release candidate is cut -@QuLogic?

@jklymak
Copy link
Member

It is milestoned 3.7, so shouldn't be a problem, right?

timhoffm and story645 reacted with thumbs up emoji

@jklymakjklymak merged commitae53649 intomatplotlib:mainSep 7, 2022
@story645
Copy link
Member

Yeah was just unsure if feature freeze comes in once the RCs are cut or if they can come in - under the wire.

Granted, targeting 3.7 gives time to roll out this new formatting support to all the functions with a fmt keyword (and support for formatters) if we want to go that route.

@timhoffm
Copy link
Member

Yeah was just unsure if feature freeze comes in once the RCs are cut or if they can come in - under the wire.

3.6.0 feature freeze comes in as soon as we have the 3.6.x branch. Then, the PRs are going into main and need to be backported to the 3.6.x brach explicitly.

story645 reacted with thumbs up emoji

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

@tacaswelltacaswelltacaswell left review comments

@story645story645story645 approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.

5 participants
@stefmolin@story645@tacaswell@timhoffm@jklymak

[8]ページ先頭

©2009-2025 Movatter.jp