-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: EngFormatter new kwarg 'sep' #6542
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: EngFormatter new kwarg 'sep' #6542
Conversation
Note to myself: don't forget to update the example once some review will have been performed. |
lib/matplotlib/ticker.py
Outdated
elif self.places > 0: | ||
format_str = ("%%.%if %%s" % self.places) | ||
format_str = "%.{p:d}f{sep:s}%s".format(p=self.places, | ||
sep=self.sep) | ||
|
||
formatted = format_str % (mant, prefix) | ||
|
||
formatted = formatted.strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this strip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And move it up to L1088 (in the new code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the solution in commit 2193c79 is more or less what you were proposing.
In fact, the strip
at the end of format_eng
was the last operation done before the fix #6014, which added the if statement that appends a trailing space when there was no prefix and unit (to fix issue #6009). It seems to me that with the new space_sep
option, this if
block is not necessary anymore and that one simply needs to perform a strip
in __call__
before returning the final formatted string: the only case where it will remove a trailing space should the one without a unit and without a prefix. The relevant test seems still OK with the new scheme.
By the way, since #6014 (and only if space_sep=True
since #6542), format_eng(0)
returns u"0 "
and not u"0"
as it is written in the docstring. So, a) is this change an issue, since format_eng
is not a private method?, b) should I fix the docstring?
lib/matplotlib/tests/test_ticker.py
Outdated
""" | ||
eng_formatter = mticker.EngFormatter(places=places, space_sep=False) | ||
# Remove the space separator from the reference cases | ||
test_cases = ((val, u"{s:s}".format(s=s.replace(" ", ""))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the string format really needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply copy-pasted from one of the previous similar cases and didn't pay more attention, but you are right: it's clearer without the string format. Removed by commit 8d086e0.
Not sure the Travis failure is related to my last commit. |
BTW, what should be done about the problem noticed l. 1134 in |
I rewrote the example about the EngFormatter in the API section ( I simply moved the demo of the kwarg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuLogic
Do the tests for this need to be re-written for pytest?
Yes, the nose stuff needs to be removed now. |
I will try to update to pytest tomorrow (“later today” may be more correct). |
aa8cbd5
to
e1421c6
Compare
I rebased my local copy on the current master (things did not go totally smooth). I removed the changes that I had made in So I will wait for CI to finish running on the rebased version before re-adding my previous changes in |
I wouldn't call it wrong, but I bet you have a new email set on github; the second icon is for the person who committed the change, rather than the author. |
As Travis seems to be fine with the rebase (and the AppVeyor failure does not look like related), I have just (re-)included to extended test of EngFormatter that I had introduced before (If I did not forget any, it should of course include all the test cases that were tested in the previous commit). I hope I translated it well in Pytest style: we will see if CI (at least Travis) is still happy with this new version! @dopplershift I think you may be write for the double thumbnail (I remind pushing stuff from computers with different email addresses). Well I guess that in the end, we will play with the mailmap file if it does mess with the statistics. |
Well, at least both CI services agree on failing \o/… From a rapid glance at the logs, it seems that I mixed some “expected outputs” in the test revamping. I will double check and take care of it. |
Why not use a half space or make the space character customizable? |
Well I think it just seemed easier to me to implement this with a boolean flag when I opened this PR (I was more or less a newcomer to mpl/git/test suite & Co). What you suggest may indeed be better from the user viewpoint. We could replace the kwarg @tacaswell @QuLogic @phobson @dopplershift (summonning every people who posted in the thread ^^) Thoughts? Who this idea be better than the current one? |
BTW, just |
👍 to 👎 to a non-ascii default, but 👍 to mentioning that in the docs. |
What wrong with an non ASCII default? |
@tacaswell Just to be sure, by non-ascii default, you mean that it should be |
I mean You should not need the leading |
…n class and init docstrings
033111f
to
ac42b94
Compare
@WeatherGod I just rebased and dropped the last commit, which introduced the use of a regex. @HDembinski Thank you for benchmarking :). |
I do not understand what is going wrong with AppVeyor :/. I already restarted it twice and it always fails at during the same build (Python 3.5) with what look like network issues
But three times in a row, it is not really intermittent to me anymore ^^... |
Marking it as release-critical because it is a feature that I would really appreciate to see in next release and I think it is finally in good shape (apart from the AppVeyor failure maybe). Feel free to remove the label if you disagree. |
🎉 |
@tacaswell Thank you :). And many thanks to all the people who got involved and reviewed this PR! |
Implements the proposition from #6533, by adding a kwarg
space_sep
toticker.EngFormatter
.The default value of this new kwarg
space_sep
isTrue
which preserves the current formatting, with a space separator between the value and the prefix/unit (like in "1.23 µV"). If it is set toFalse
, this space separator is removed (like in "1.23µV"). The latter is not really correct from the typographic viewpoint but will (for example) allow users who don't really care to save space in the ticklabels. As discussed in #6533, when no unit symbol is provided, "1.23 µ" and "1.23µ" seem to be both wrong (at least for typographers), so I chose to keep the current behavior as default.The commit 0f37c41 fixes a minor inconsistency between different representations of zero. Now,
formatter(-0.0) == formatter(0) == formatter(-0) == u"0"
(withformatter = EngFormatter()
), while beforeformatter(-0) = u"0"
butformatter(-0.0) = u"-0"
for example...The commit 25cdf5f is a rewrite of the unit test
test_EngFormatter_formatting()
I previously added in PR #6014 . The new test now tests all the possible combinations of the kwargs, which was not the case before (in particular, only the defaultplaces=None
was tested). Furthermore, the tested values now exercise a bigger range of cases (like for example 0, -0 and -0.0).And by the way, I tried to improve a few docstrings. Hopefully I didn't add to many English mistakes or typos...