ENH: Added a `PercentFormatter` class to `matplotlib.ticker` by madphysicist · Pull Request #6251 · matplotlib/matplotlib

madphysicist · 2016-03-30T21:03:48Z

This is suitable for formatting axes labels as percentages. It has some nice features like being able to convert from arbitrary data scales to percents, a customizable percent symbol and either automatic or manual control over the decimal points.

The original motivation came from my own work as well as these two Stack Overflow questions:

madphysicist · 2016-03-30T21:05:03Z

@mdboom While I realize that you have a script to do this, it is really disconcerting to see your name pop up fractions of a second after I create a PR :)

efiring · 2016-03-30T21:23:58Z

lib/matplotlib/ticker.py

+        suitable for use with single-letter representations of powers of
+        1000. For example, 'Hz' or 'm'.
+
+        `places` is the percision with which to display the number,


madphysicist · 2016-03-30T21:28:27Z

I am not familiar enough with Python 2.7 to immediately understand why the following is not working:

s = '{x:0.{decimals}f}'.format(x=x, decimals=decimals)

Are nested formats not allowed?

efiring · 2016-03-30T21:28:46Z

lib/matplotlib/ticker.py

+    already scaled to be percentages, `mx` will be 100. Another common
+    situation is where `max` is 1.0.
+    """
+    def __init__(self, mx=100, decimals=None, symbol='%'):


I think people will rarely want to specify max, so I would put it after the decimals kwarg.

I disagree. The most common parameter to modify would be max. This is intended for data going from 0 to 1, or as in the case of the SO question, from 0 to 5. I would expect the decimals argument to generally remain None assuming I have done a decent job with the automated version.

efiring · 2016-03-30T21:31:19Z

There are so many cleanup changes here that I recommend breaking this into two PRs: one for the cleanups, and a second one for the new feature.

madphysicist · 2016-03-30T21:34:22Z

No problem with that. Is there an easy way to do that without closing this PR?

efiring · 2016-03-30T21:37:13Z

lib/matplotlib/ticker.py

        1000. For example, 'Hz' or 'm'.

-        `places` is the percision with which to display the number,
+        `places` is the prrcision with which to display the number,


QuLogic · 2016-03-30T21:38:05Z

@mdboom While I realize that you have a script to do this, it is really disconcerting to see your name pop up fractions of a second after I create a PR :)

That's https://waffle.io/

No problem with that. Is there an easy way to do that without closing this PR?

Create a new branch starting on this one, rebase it to include only cleanup, and open a new PR for it. When that's merged, rebase this one on top of master and drop all the cleanup from here.

efiring · 2016-03-30T21:41:19Z

I think you would make a separate branch on your repo for the cleanups, and make a PR from that. Then, on your pctFormatter branch, isolate the PercentFormatter part with additional commits, and use 'git rebase -i' to squash it down to a single commit. Force-push that to your github repo, and it will replace the present commits in the present PR.

madphysicist · 2016-03-31T05:07:57Z

I did the split (#6253) and fixed the Py2.7 error. Turns out Py3 automatically converts acceptable format precisions into an int. Py2 does not.

The only potentially unrelated change I kept here was moving __all__ to the top.

madphysicist · 2016-03-31T06:25:02Z

Metabolized the docs of EngFormatter.format_eng from #6253 into this commit to get rid of sphinx errors.

madphysicist · 2016-04-01T20:19:11Z

I think that this PR is complete (ready for further review). Same goes for #6253.

The failure in Appveyor is the sporadic image comparison failure. It is not related to the code I added as far as I can tell.

madphysicist · 2016-04-07T18:37:55Z

Squawk.

mdboom · 2016-04-07T18:46:26Z

Cc: @efiring.

efiring · 2016-04-07T21:13:32Z

In reading the new code, I find that I waste a lot of time trying to figure out what d really is. I am comfortable with short variable names like x when their meaning is obvious from the context; but d is not obvious here--especially since some places it is in unscaled data units, and other places in percent. So maybe use dspan or xspan, and pcspan, or something like that? And maybe xmax instead of mx? Readability matters, both in the tests and in the active code.

madphysicist · 2016-04-08T13:39:18Z

I changed max to xmax throughout, and changed d to display_range and scaled_range as appropriate. I think that does make a big improvement to readability.

On a side-note, I got d from OldScalarFormatter, originally thinking to reuse its pprint method. I don't think that d needs to be changed at this point, but it is a possibility. d also appears in at least one of the Locators.

efiring · 2016-04-08T16:18:32Z

lib/matplotlib/tests/test_ticker.py

+        (42, None, '^^Foobar$$', 21, 12, '50.0^^Foobar$$'),
+    )
+    for xmax, decimals, symbol, x, display_range, expected in test_cases:
+        yield _percent_format_helper, xmax, decimals, symbol, x, display_range, expected


pep-8 failure here: line too long. You could use parentheses; or you could use

for case in test_cases: yield (_percent_format_helper,) + case

I haven't tested, but I think that would work and would be more readable.
Maybe I would use args and test_args instead of case and test_cases.

efiring · 2016-04-08T16:30:27Z

In addition to the pep-8 failure noted above there is a real failure that I have not tracked down (probably you will spot it instantly). Once those are taken care of, I think this will be functionally ready. It's a nice contribution. You could neaten it up by rebasing to squash the commits.

madphysicist · 2016-04-08T16:30:53Z

I went with

for case in test_cases:
    yield (_percent_format_helper, *case)

and also fixed the other stupid typo. Will push with squash soon.

madphysicist · 2016-04-08T16:51:18Z

And now I know why I should have listened the first time.

madphysicist · 2016-04-08T17:35:43Z

I don't think the Travis failure is related to my code.

efiring · 2016-04-08T20:35:21Z

Thank you, @madphysicist!

madphysicist · 2016-04-08T20:43:39Z

No problem. Thanks for looking over and accepting my PR!

mdboom added the status: needs review label Mar 30, 2016

efiring reviewed Mar 30, 2016
View reviewed changes

madphysicist mentioned this pull request Mar 31, 2016

MAINT: Updates to formatters in matplotlib.ticker #6253

Merged

madphysicist force-pushed the pctFormatter branch from 3318d77 to 06d6f41 Compare March 31, 2016 05:05

madphysicist changed the title ~~Added a PercentFormatter class to matplotlib.ticker~~ ENH: Added a PercentFormatter class to matplotlib.ticker Mar 31, 2016

madphysicist changed the title ~~ENH: Added a PercentFormatter class to matplotlib.ticker~~ ENH: Added a PercentFormatter class to matplotlib.ticker Mar 31, 2016

madphysicist force-pushed the pctFormatter branch 3 times, most recently from 94a8859 to fbb8eb4 Compare March 31, 2016 06:23

madphysicist force-pushed the pctFormatter branch 2 times, most recently from c91a87b to f123047 Compare March 31, 2016 17:32

tacaswell added this to the 2.1 (next point release) milestone Apr 7, 2016

madphysicist force-pushed the pctFormatter branch from f123047 to 5161407 Compare April 8, 2016 13:29

madphysicist added a commit to madphysicist/matplotlib that referenced this pull request Apr 8, 2016

MAINT: Response to review comments to matplotlib#6251

5161407

efiring reviewed Apr 8, 2016
View reviewed changes

madphysicist force-pushed the pctFormatter branch from 27cf9a5 to 02f61c0 Compare April 8, 2016 16:33

ENH: Added percent formatter and tests

0e35e6b

madphysicist force-pushed the pctFormatter branch from 02f61c0 to 0e35e6b Compare April 8, 2016 16:53

efiring merged commit c03262a into matplotlib:master Apr 8, 2016

efiring removed the status: needs review label Apr 8, 2016

madphysicist deleted the pctFormatter branch April 8, 2016 20:35

madphysicist mentioned this pull request Nov 17, 2016

TST: Fixed part of a test that got displaced in all the changes somehow #7467

Merged

Uh oh!

Conversation

madphysicist commented Mar 30, 2016

Uh oh!

madphysicist commented Mar 30, 2016

Uh oh!

efiring Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

madphysicist commented Mar 30, 2016

Uh oh!

efiring Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

madphysicist Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

efiring commented Mar 30, 2016

Uh oh!

madphysicist commented Mar 30, 2016

Uh oh!

efiring Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Mar 30, 2016

Uh oh!

efiring commented Mar 30, 2016

Uh oh!

madphysicist commented Mar 31, 2016

Uh oh!

madphysicist commented Mar 31, 2016

Uh oh!

madphysicist commented Apr 1, 2016

Uh oh!

madphysicist commented Apr 7, 2016

Uh oh!

mdboom commented Apr 7, 2016

Uh oh!

efiring commented Apr 7, 2016

Uh oh!

madphysicist commented Apr 8, 2016

Uh oh!

efiring Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

efiring commented Apr 8, 2016

Uh oh!

madphysicist commented Apr 8, 2016

Uh oh!

madphysicist commented Apr 8, 2016

Uh oh!

madphysicist commented Apr 8, 2016

Uh oh!

efiring commented Apr 8, 2016

Uh oh!

madphysicist commented Apr 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments