-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
MAINT: Updates to formatters in matplotlib.ticker
#6253
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
MAINT: Updates to formatters in matplotlib.ticker
#6253
Conversation
dc83624
to
c77fe94
Compare
""" | ||
Return a short string version of the tick value. | ||
|
||
Defaults to the position-indepedent long value. |
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.
independent
c07d694
to
7602b04
Compare
Switch minor tick labeling on or off. | ||
|
||
``labelOnlyBase=True`` to turn off minor ticks. | ||
""" |
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.
Does this currently work? Should it have a not
in there?
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.
This particular method I have no idea. The formatter itself seems to work OK though. I will check right now.
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.
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
?
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.
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.
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.
Makes sense, will do.
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 have opened up #6264 with the code change.
This looks good, but needs a rebase 😞 \ |
This should not break backwards compatibility since extra keywords are allowed
Moved to the top and added missing formatter classes. Probably missing some locators too, but that's for the future.
7602b04
to
29b2ea6
Compare
Fixed. I've been spoiled by Homu on numpy. |
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
backported to v2.x as 668f292 |
This PR is a split from #6251. Most of the changes are to docs, but there are two minor code changes:
__all__
has been moved to the top of the file and a couple of missing formatters were added to it.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.