MAINT: Updates to formatters in `matplotlib.ticker` #6253

Merged
merged 7 commits into from Apr 28, 2016

Conversation

Projects
None yet
4 participants
Contributor

madphysicist commented Mar 31, 2016

This PR is a split from #6251. Most of the changes are to docs, but there are two minor code changes:

  1. __all__ has been moved to the top of the file and a couple of missing formatters were added to it.
  2. A second format (position) is permitted in the string for the new-style string formatter.

As the title suggests, only formatters were affected. Locators were left untouched.

I have retained the original commits until the PR is reviewed to make sure that I did the split correctly.

mdboom added the needs_review label Mar 31, 2016

@QuLogic QuLogic commented on an outdated diff Mar 31, 2016

lib/matplotlib/ticker.py
return self.__call__(value)
def format_data_short(self, value):
- """return a short string version"""
+ """
+ Return a short string version of the tick value.
+
+ Defaults to the position-indepedent long value.
@QuLogic

QuLogic Mar 31, 2016

Member

independent

@QuLogic QuLogic commented on an outdated diff Mar 31, 2016

lib/matplotlib/ticker.py
return self.func(x, pos)
class FormatStrFormatter(Formatter):
"""
- Use an old-style ('%' operator) format string to format the tick
+ Use an old-style ('%' operator) format string to format the tick.
+
+ The format string should have a single variable format (%) in it.
+ It will be applied to the value (not the postition) of the tick.
@QuLogic

QuLogic Mar 31, 2016

Member

position

@QuLogic QuLogic and 1 other commented on an outdated diff Mar 31, 2016

lib/matplotlib/ticker.py
def __init__(self, useOffset=None, useMathText=None, useLocale=None):
+ """
@QuLogic

QuLogic Mar 31, 2016

Member

Does it make sense to document both class and __init__? I think Sphinx has a setting for it, so it may ignore one or the other, or even just combine both together.

@madphysicist

madphysicist Mar 31, 2016

Contributor

Good point. Merged into class description. It is more likely to get picked up and put in the right place in case there is a difference.

@QuLogic QuLogic commented on an outdated diff Mar 31, 2016

lib/matplotlib/ticker.py
Sets size thresholds for scientific notation.
- e.g., ``formatter.set_powerlimits((-3, 4))`` sets the pre-2007 default
- in which scientific notation is used for numbers less than 1e-3 or
- greater than 1e4.
- See also :meth:`set_scientific`.
- '''
+ Limits is a two-element sequence containing the powers of 10
@QuLogic

QuLogic Mar 31, 2016

Member

What's Limits? There's no argument with that name.

@tacaswell tacaswell commented on the diff Apr 2, 2016

lib/matplotlib/ticker.py
self._base = base
def label_minor(self, labelOnlyBase):
- 'switch on/off minor ticks labeling'
+ """
+ Switch minor tick labeling on or off.
+
+ ``labelOnlyBase=True`` to turn off minor ticks.
+ """
@tacaswell

tacaswell Apr 2, 2016

Owner

Does this currently work? Should it have a not in there?

@madphysicist

madphysicist Apr 2, 2016

Contributor

This particular method I have no idea. The formatter itself seems to work OK though. I will check right now.

@madphysicist

madphysicist Apr 2, 2016

Contributor

This method does nothing for this particular class since self.labelOnlyBase is not used in self.pprint_val. However, it does work for the LogFormatterExponent class, which extends this one. The old docs and the method name are indeed very misleading. labelOnlyBase is named correctly (as I have noted in the updated docs). If it is True, minor ticks will not be shown, which is exactly the opposite of what the name of the method implies. I did not find any explicit calls to this method in matplotlib. Am I free to modify the parameter name to say labelMinor and set self.labelOnlyBase = not labelMinor?

@tacaswell

tacaswell Apr 2, 2016

Owner

Yes, but can you do that in a separate PR? The doc updates are non-controversial and should go in, but the API change should get more attention.

@madphysicist

madphysicist Apr 2, 2016

Contributor

Makes sense, will do.

@madphysicist

madphysicist Apr 3, 2016

Contributor

I have opened up #6264 with the code change.

Owner

tacaswell commented Apr 11, 2016

This looks good, but needs a rebase 😞 \

madphysicist added some commits Mar 29, 2016

@madphysicist madphysicist MAINT: Updated mailmap eeb7658
@madphysicist madphysicist ENH: Added support for both x and pos to StrMethodFormatter
This should not break backwards compatibility since extra keywords are allowed
9ba1428
@madphysicist madphysicist DOC: Updated docs for formatters in ticker 89a1ba1
@madphysicist @madphysicist madphysicist TST: Test for new pos format 9cb7ee8
@madphysicist madphysicist MAINT: Fixed up __all__ in ticker
Moved to the top and added missing formatter classes.
Probably missing some locators too, but that's for the future.
864a6cc
@madphysicist @madphysicist madphysicist DOC: Wrapped to 72 chars and fixed a couple of missed spots c94d0b1
@madphysicist @madphysicist madphysicist Responses to PR#6253
29b2ea6
Contributor

madphysicist commented Apr 11, 2016

Fixed. I've been spoiled by Homu on numpy.

@tacaswell tacaswell merged commit 8658721 into matplotlib:master Apr 28, 2016

2 checks passed

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

tacaswell removed the needs_review label Apr 28, 2016

@tacaswell tacaswell added a commit that referenced this pull request Apr 28, 2016

@tacaswell tacaswell Merge pull request #6253 from madphysicist/ticker-formatter-maint
MAINT: Updates to formatters in `matplotlib.ticker`
Conflicts:
	.mailmap
	  - just added the 1 new line, did not change any others
	lib/matplotlib/tests/test_ticker.py
	  - whitespace vs new tests
668f292
Owner

tacaswell commented Apr 28, 2016

backported to v2.x as 668f292

madphysicist deleted the madphysicist:ticker-formatter-maint branch May 1, 2016

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