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

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

Conversation

afvincent
Copy link
Contributor

Implements the proposition from#6533, by adding a kwargspace_sep toticker.EngFormatter.

The default value of this new kwargspace_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 testtest_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...

@afvincentafvincent changed the titleEnh engformatter space sep new optionENH: engformatter space sep new optionJun 6, 2016
@afvincentafvincent changed the titleENH: engformatter space sep new optionENH: EngFormatter space_sep new optionJun 6, 2016
@afvincent
Copy link
ContributorAuthor

Note to myself: don't forget to update theexample once some review will have been performed.

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()
Copy link
Member

Choose a reason for hiding this comment

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

I would remove thisstrip

Copy link
Member

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)

Copy link
ContributorAuthor

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?

@tacaswelltacaswell added this to the2.1 (next point release) milestoneJun 6, 2016
"""
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(" ", "")))
Copy link
Member

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?

Copy link
ContributorAuthor

@afvincentafvincentJun 7, 2016
edited
Loading

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.

@afvincent
Copy link
ContributorAuthor

Not sure the Travis failure is related to my last commit.

@afvincent
Copy link
ContributorAuthor

BTW, what should be done about the problem noticed l. 1134 inticker.py (# TODO: shouldn't we raise a warning if self.places < 0?)? Should an explicite exception be raised, or do we assume users won't call an instance of EngFormatter with a negative value for the kwargplaces.

@afvincent
Copy link
ContributorAuthor

I rewrote the example about the EngFormatter in the API section (engineering_formatter.py) to demonstrate about the new optionspace_sep. Eventhough most of the lines have been modified, it is the same spirit and data as before (the former example is kept as left panel).

I simply moved the demo of the kwargplaces from the former example (left panel) to the new one (right panel) for esthetical reasons: withplaces=1 in the left panel, the tick labels were a bit mixed.

Here is a example how it now looks like:
updated_example

Copy link
Member

@phobsonphobson left a 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?

@QuLogic
Copy link
Member

Yes, the nose stuff needs to be removed now.

@afvincentafvincent self-assigned thisFeb 10, 2017
@afvincent
Copy link
ContributorAuthor

I will try to update to pytest tomorrow (“later today” may be more correct).

@afvincentafvincentforce-pushed theenh_engformatter_space_sep_new_option branch fromaa8cbd5 toe1421c6CompareFebruary 11, 2017 12:22
@afvincent
Copy link
ContributorAuthor

I rebased my local copy on the current master (things did not go totally smooth). I removed the changes that I had made intest_ticker because they were conflicting with the ones made during the Pytest migration. And of course, I have issues with Pytest when I want to check if I broke anything (it is complaining aboutcycler, which is installed and up-to-date on my system though).

So I will wait for CI to finish running on the rebased version before re-adding my previous changes intest_ticker, converted to Pytest.

@afvincent
Copy link
ContributorAuthor

Huh, what did I do wrong withrebase to get twice my GitHub thumbnail with commits43d812d,9121205,a1a38c5 and7bad242 ^^?

@dopplershift
Copy link
Contributor

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.

@afvincent
Copy link
ContributorAuthor

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.

@afvincent
Copy link
ContributorAuthor

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.

@Tillsten
Copy link
Contributor

Why not use a half space or make the space character customizable?

@afvincent
Copy link
ContributorAuthor

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 kwargspace_sep=True(default)|False withseparator=u" " (default)|any string.Because backward compatibility is something we worship, the new default would still have to be a full space (not even unbreakable :/) as it is the original behavior. However, you could remove the space separator withseparator="" or even use a narrow no-break white space withseparator=u'\u202f' if you prefer.

@tacaswell@QuLogic@phobson@dopplershift (summonning every people who posted in the thread ^^) Thoughts? Who this idea be better than the current one?

@afvincent
Copy link
ContributorAuthor

BTW, justsep may be a better name for a customizable separator argument. For example, I just discovered thatprint also has a separator argument, calledsep. (Always learning…)

@tacaswell
Copy link
Member

👍 tosep as the kwarg name and 👍 to it being configurable rather than just a boolean.

👎 to a non-ascii default, but 👍 to mentioning that in the docs.

@Tillsten
Copy link
Contributor

What wrong with an non ASCII default?

@afvincent
Copy link
ContributorAuthor

@tacaswell Just to be sure, by non-ascii default, you mean that it should besep=" " instead ofsep=u" "?

@tacaswell
Copy link
Member

I mean" " instead of'\u202f'.

You should not need the leadingu anywhere. We haveunicode_literals at the top of all of our files. We no longer support 3.2 so they are notforbidden anymore, but just not needed.

@afvincentafvincentforce-pushed theenh_engformatter_space_sep_new_option branch from033111f toac42b94CompareAugust 25, 2017 16:40
@afvincent
Copy link
ContributorAuthor

@WeatherGod I just rebased and dropped the last commit, which introduced the use of a regex.

@HDembinski Thank you for benchmarking :).

@afvincent
Copy link
ContributorAuthor

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

CondaError: CondaHTTPError: HTTP 000 CONNECTION FAILED for url <https://repo.continuum.io/pkgs/free/win-64/mkl-2017.0.3-0.tar.bz2>Elapsed: -An HTTP error occurred when trying to retrieve this URL.HTTP errors are often intermittent, and a simple retry will get you on your way.

But three times in a row, it is not really intermittent to me anymore ^^...

@afvincentafvincent added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelAug 26, 2017
@afvincent
Copy link
ContributorAuthor

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.

@tacaswelltacaswell merged commitdf6acf9 intomatplotlib:masterAug 26, 2017
@tacaswell
Copy link
Member

🎉

@afvincent
Copy link
ContributorAuthor

@tacaswell Thank you :).

And many thanks to all the people who got involved and reviewed this PR!

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

@tacaswelltacaswelltacaswell approved these changes

@QuLogicQuLogicQuLogic left review comments

@phobsonphobsonphobson left review comments

@anntzeranntzeranntzer approved these changes

Assignees

@afvincentafvincent

Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.

10 participants
@afvincent@QuLogic@dopplershift@Tillsten@tacaswell@NelleV@anntzer@WeatherGod@phobson@mdboom

[8]ページ先頭

©2009-2025 Movatter.jp