ENH: Added a `PercentFormatter` class to `matplotlib.ticker` #6251

Merged
merged 1 commit into from Apr 8, 2016

Conversation

Projects
None yet
5 participants
Contributor

madphysicist commented Mar 30, 2016

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:

mdboom added the needs_review label Mar 30, 2016

Contributor

madphysicist commented Mar 30, 2016

@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 efiring commented on an outdated diff Mar 30, 2016

lib/matplotlib/ticker.py
@@ -914,6 +1009,17 @@ class EngFormatter(Formatter):
}
def __init__(self, unit="", places=None):
+ """
+ Initializes an engineering notation formatter.
+
+ `unit` is a string containing the abbreviated name of the unit,
+ 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,
@efiring

efiring Mar 30, 2016

Owner

precision

@efiring efiring commented on an outdated diff Mar 30, 2016

lib/matplotlib/ticker.py
+:class:`EngFormatter`
+ Format labels in engineering notation
+
+:class:`PerentFormatter`

@efiring efiring commented on an outdated diff Mar 30, 2016

lib/matplotlib/ticker.py
@@ -973,6 +1078,87 @@ def format_eng(self, num):
return formatted.strip()
+class PercentFormatter(Formatter):
+ """
+ Format numbers as a percentage.
+
+ How the number is converted into a percentage is determined by the
+ `mx` parameter. `mx` is the data value that corresponds to 100%.
+ Percentages are computed as ``x / mx * 100``. So if the data is
+ 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='%'):
+ """
+ Initializes the formatter.
+
+ `max` is the data value that corresponds to 100%. `symbol` is
@efiring

efiring Mar 30, 2016

Owner

This needs to match the name in the signature (i.e., 'max' needs to be the kwarg).

Contributor

madphysicist commented Mar 30, 2016

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 efiring and 1 other commented on an outdated diff Mar 30, 2016

lib/matplotlib/ticker.py
@@ -973,6 +1078,87 @@ def format_eng(self, num):
return formatted.strip()
+class PercentFormatter(Formatter):
+ """
+ Format numbers as a percentage.
+
+ How the number is converted into a percentage is determined by the
+ `mx` parameter. `mx` is the data value that corresponds to 100%.
+ Percentages are computed as ``x / mx * 100``. So if the data is
+ 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='%'):
@efiring

efiring Mar 30, 2016

Owner

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

@madphysicist

madphysicist Mar 30, 2016

Contributor

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 efiring and 1 other commented on an outdated diff Mar 30, 2016

lib/matplotlib/ticker.py
+ d = self.convert_to_pct(d) # d is a difference, so this works fine
+ decimals = math.ceil(2.0 - math.log10(2.0 * d))
+ if decimals > 5:
+ decimals = 5
+ elif decimals < 0:
+ decimals = 0
+ else:
+ decimals = self.decimals
+ s = '{x:0.{decimals}f}'.format(x=x, decimals=decimals)
+
+ if self.symbol:
+ return s + self.symbol
+ return s
+
+ def convert_to_pct(self, x):
+ return x / self.max * 100.0
@efiring

efiring Mar 30, 2016

Owner

I like to use parentheses to make expressions like this clearer; or multiply by 100 first.

@madphysicist

madphysicist Mar 30, 2016

Contributor

Doing both. I agree that it looks awkward.

Owner

efiring commented Mar 30, 2016

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.

Contributor

madphysicist commented Mar 30, 2016

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

@efiring efiring commented on an outdated diff Mar 30, 2016

lib/matplotlib/ticker.py
@@ -1016,7 +1016,7 @@ def __init__(self, unit="", places=None):
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,
+ `places` is the prrcision with which to display the number,
Member

QuLogic commented Mar 30, 2016

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

Owner

efiring commented Mar 30, 2016

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.

Contributor

madphysicist commented Mar 31, 2016

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 changed the title from Added a PercentFormatter class to `matplotlib.ticker` to ENH: Added a PercentFormatter class to `matplotlib.ticker` Mar 31, 2016

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

Contributor

madphysicist commented Mar 31, 2016

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

Contributor

madphysicist commented Apr 1, 2016

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.

Contributor

madphysicist commented Apr 7, 2016

Squawk.

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

Owner

mdboom commented Apr 7, 2016

Cc: @efiring.

Owner

efiring commented Apr 7, 2016

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 madphysicist added a commit to madphysicist/matplotlib that referenced this pull request Apr 8, 2016

@madphysicist madphysicist MAINT: Response to review comments to #6251 5161407
Contributor

madphysicist commented Apr 8, 2016

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 efiring commented on an outdated diff Apr 8, 2016

lib/matplotlib/tests/test_ticker.py
+ (100, 1, '%', 70.23, 50, '70.2%'),
+ # 60.554 instead of 60.55: see https://bugs.python.org/issue5118
+ (100, 1, '%', 60.554, 40, '60.6%'),
+ # Check auto decimals over different intervals and values
+ (100, None, '%', 95, 1, '95.00%'),
+ (1.0, None, '%', 3, 6, '300%'),
+ (17.0, None, '%', 1, 8.5, '6%'),
+ (17.0, None, '%', 1, 8.4, '5.9%'),
+ (5, None, '%', -100, 0.000001, '-2000.00000%'),
+ # Check percent symbol
+ (1.0, 2, None, 1.2, 100, '120.00'),
+ (75, 3, '', 50, 100, '66.667'),
+ (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
@efiring

efiring Apr 8, 2016

Owner

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.

Owner

efiring commented Apr 8, 2016

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.

Contributor

madphysicist commented Apr 8, 2016

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.

Contributor

madphysicist commented Apr 8, 2016

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

@madphysicist madphysicist ENH: Added percent formatter and tests
0e35e6b
Contributor

madphysicist commented Apr 8, 2016

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

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

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

efiring removed the needs_review label Apr 8, 2016

Owner

efiring commented Apr 8, 2016

Thank you, @madphysicist!

madphysicist deleted the madphysicist:pctFormatter branch Apr 8, 2016

Contributor

madphysicist commented Apr 8, 2016

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

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