Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
ENH: EngFormatter new kwarg 'sep'#6542
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Note to myself: don't forget to update theexample 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 thisstrip
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 commit2193c79 is more or less what you were proposing.
In fact, thestrip
at the end offormat_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 newspace_sep
option, thisif
block is not necessary anymore and that one simply needs to perform astrip
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 ifspace_sep=True
since#6542),format_eng(0)
returnsu"0 "
and notu"0"
as it is written in the docstring. So, a) is this change an issue, sinceformat_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 commit8d086e0.
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
toe1421c6
CompareI 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
toac42b94
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 commit0f37c41 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 commit25cdf5f 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...