Patch for issue #6009 #6014

Merged
merged 7 commits into from Jun 6, 2016

Conversation

Projects
None yet
6 participants
Contributor

afvincent commented Feb 17, 2016

Here is a patch for issue #6009 about the EngFormatter in matplotlib.ticker.

It simply enforces a space when there is no SI prefix but yet a unit symbol. For example “10 seconds” should be formatted as “10 s” by the EngFormatter (previously “10s”).

mdboom added the needs_review label Feb 17, 2016

Member

WeatherGod commented Feb 17, 2016

Should add a unit test to exercise this. Preferably not an image test (we have plenty of those), but one that tests the strings coming out of EngFormatter.

tacaswell added this to the 2.1 (next point release) milestone Feb 18, 2016

@tacaswell tacaswell commented on an outdated diff Feb 18, 2016

lib/matplotlib/ticker.py
@@ -960,7 +960,11 @@ def format_eng(self, num):
formatted = format_str % (mant, prefix)
- return formatted.strip()
+ formatted = formatted.strip()
+ if (self.unit is not "") and (prefix is self.ENG_PREFIXES[0]):
@tacaswell

tacaswell Feb 18, 2016

Owner

I think this should be == not is. It is just a little less brittle.

Owner

tacaswell commented Feb 18, 2016

Left minor comment, but other wise 👍

Contributor

afvincent commented Feb 18, 2016

I don't understand why my last commit broke the Travis CI build test: it seems to me that the changes are really minor, aren't they? Furthermore, when looking at the failure logs in “Details” section of Travis, one can find errors like FAIL: matplotlib.tests.test_colorbar.test_colorbar_closed_patch.test and all the 6 (unexpected) failures relate to test_colorbar. How can it be impacted by a change in ticker.EngFormatter?

Owner

jenshnielsen commented Feb 18, 2016

Travis is broken because the merge of another pr broke master is is not caused by your pr

Contributor

afvincent commented Feb 18, 2016

Ok, thank you for the explanation :).

Owner

tacaswell commented Jun 4, 2016

Can you rebase so we can merge it?

Contributor

afvincent commented Jun 5, 2016

I think I've done a mistake on this branch with my git local repo (what a change…). I used the same branch to implement the enhancement I presented in #6533 (which also fix this issue btw). Should I go back in time with git and then rebase, or do something else? I guess opening a new PR (on a branch from an up-to-date master) which fixes this small issue and also implements the space_sep new option isn't really a clean option, is it?

afvincent added some commits Feb 17, 2016

@afvincent @afvincent afvincent Update ticker.py 6813d84
@afvincent @afvincent afvincent Unit test on strings coming out of EngFormatter
Hope it is what was expected when asking for some unit (non image) test.
6f9bf5f
@afvincent @afvincent afvincent Correct name for ticker in EngFormatter test
Sorry, I was using a different name than “mticker” to import the ticker module, and I had forgotten to change it when committing the new unit test test_EngFormatter_formatting()…
ca341a5
@afvincent @afvincent afvincent PEP8 compliance
Now “All right” on pep8online.com for the method format_eng in the class EngFormatter.
cf20132
@afvincent @afvincent afvincent New versions of (non) equality statements 7724f09
@afvincent afvincent Minor fix in the docstring.
6d584ad
Contributor

afvincent commented Jun 5, 2016

I rebased after deleting my last commit (the one with the enhancement #6533). Hopefully it will be OK.

@afvincent afvincent Fix a minor typo in EngFormatter docstring
2ab06c5

@tacaswell tacaswell merged commit 0ac187d into matplotlib:master Jun 6, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 69.656%
Details

tacaswell removed the needs_review label Jun 6, 2016

@tacaswell tacaswell added a commit that referenced this pull request Jun 6, 2016

@tacaswell tacaswell Merge pull request #6014 from afvincent/afvincent-patch-issue-6009
FIX: EngFormatter without but without prefix

closes #6009
59e0ab9
Owner

tacaswell commented Jun 6, 2016 edited

backported to v2.x as 59e0ab9

afvincent deleted the afvincent:afvincent-patch-issue-6009 branch Jun 6, 2016

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