-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: Added a new Formatter to mpl.ticker and some example recipes #7482
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
Conversation
aee6c0e
to
db56c6a
Compare
db56c6a
to
53893d0
Compare
What would be the use case for the binary formatter and the hex formatter? |
I also don't understand what the linearscale formatter does. Can you add an example? |
I developed the Binary and Hex formatters specifically for plotting the output of a Digital-to-Analog Converters versus the input. The x-axis needed to indicate the input bits in a way that was easier to understand than decimal numbers. LinearScaleFormatter is a way of transforming the values shown on the axis without modifying the underlying data. It can probably be generalized to an arbitrary tick value-to-display value mapping function, but I found it useful to just specify a linear transformation with two points on the input range and two points on the output range. A sample use-case would be if you had data acquired with say imperial units like quarts, but wanted to display the axis in liters. If you did not want to provide a transformed dataset, you could wrap your existing formatter with something like |
Both the binary and hex formatters could be implemented with (or replaced by?) the The example of changing units is something we should get more unit support for, though I feel like much of the underlying infrastructure is there already, but I'm not sure how to use it. Maybe one of the unit examples would be useful. |
I thought so too at first, but it turns out you have to explicitly cast to an int to use |
Really strange that it's not defined because you can do |
Yeah. Python 3.5.2:
|
What I can do is redefine |
3c5a838
to
103ead1
Compare
@QuLogic I have converted @NelleV Hopefully the new form is more acceptable. While I agree that the original implementation was a bit domain specific, a formatter that handles the conversion to integer properly is much more generally useful. I will get the bugs out shortly. |
For the sake of avoiding adding more very thin classes, I'd suggest simply adding a doc entry suggesting the use of
In any case, please make all attributes of these new classes private, unless you have a strong reason to keep them public. Also, please check how |
Current coverage is 61.97% (diff: 87.09%)@@ master #7482 diff @@
==========================================
Files 173 174 +1
Lines 56140 56059 -81
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 34743 34743
+ Misses 21397 21316 -81
Partials 0 0
|
@anntzer. While I agree that I will rewrite Finally, when you say "making the attributes private", do you mean prepending an underscore or something else? |
Not sure how the unit infrastructure would help since |
Oh, I completely misunderstood what "unsigned" does, sorry about that. It's arguably a bit more complicated but this can still be implemented as
which is not necessarily more legible, but now the argument is instead that this feature may be a bit too specialized? (Again I'm all for adding it in the docs.) And yes I meant that attributes must be prepended with an underscore. Otherwise it basically becomes impossible later to modify the class while maintaining backcompatibility, as users may be relying on whatever public attributes are present... |
OK. I will make all the object attributes private. I would like to keep the |
lib/matplotlib/ticker.py
Outdated
'Locator', 'IndexLocator', 'FixedLocator', 'NullLocator', | ||
'LinearLocator', 'LogLocator', 'AutoLocator', | ||
'LinearScaleFormatter', 'BinaryIntFormatter', | ||
'HexIntFormatter', 'Locator', 'IndexLocator', 'FixedLocator', |
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.
There are non-existent classes in here.
lib/matplotlib/ticker.py
Outdated
@@ -154,6 +154,15 @@ | |||
:class:`PercentFormatter` | |||
Format labels as a percentage | |||
|
|||
:class:`BinaryIntFormatter` |
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.
These docs have gotten out of sync with the PR.
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.
That last push was mostly an accident. I am overhauling the code based on @anntzer's advice. Thinking about turning some of it into recipes in the docs.
lib/matplotlib/ticker.py
Outdated
This formatter can use any other formatter to actually render the | ||
ticks. | ||
|
||
inRef: number or 2-element iterable |
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.
nitpick, there should be a space before the :
to format correctly with numpydoc
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.
The last input to this is already a formatter instance, maybe take in a Norm
instance as the input here?
If not, I have a slight preference to 4 inputs instead of 2 of mixed type.
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.
Norm
instance?
lib/matplotlib/ticker.py
Outdated
|
||
# All these values are retained for debugging/extension. | ||
# Only the minima are used explicitly. | ||
self.iMin, self.iMax = unpack(inRef, 'in') |
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.
The style tends to be camel case for classes, _
for everything else.
@tacaswell I believe that my latest push addresses your immediate concerns. |
@anntzer , in reference to your request
If you meant the default implementation of |
cc485ae
to
cf6852c
Compare
I took all of the advice I got on this thread and made a completely new class called The only major comment that I am no longer following here is that the two attributes of this new class are both public. I think that in this context it makes sense to do it this way, in the same way that Another possible problem is that this class is sensitive to changes in the underlying API. This is probably fine, but I want to add a note to add the appropriate redirects in Finally, I have a purely technical question. At the moment, the |
0bfbb9b
to
ea28935
Compare
Could someone please explain why the appveyor test is failing while the same configuration in Travis is passing? Is it something I am missing that can be easily fixed or should I change the test to avoid using unicode strings? |
@madphysicist Because windows ;) |
7ea51e7
to
7b05223
Compare
7b05223
to
4356fec
Compare
04469ce
to
904bd51
Compare
As far as I can tell, I have addressed all the issues with this PR. Still waiting on approval from @tacaswell and at least one other reviewer though. |
Bump. |
- both ends are missing: set ranges to 1.0 | ||
- one end is missing: set it to the other end | ||
""" | ||
self._in_offset = 0.0 if in_start is None else in_start |
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.
why aren't you setting in_start=0 and out_start=0 in the init(...)?
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 want to be able to test if the parameter was set or not. None
allows me to implement all the combinations listed in the docstring. Setting to a number would not allow that.
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.
Nevermind. I do see your point. Will fix.
self._in_scale = out_end - self._in_offset | ||
else: | ||
self._in_scale = in_end - self._in_offset | ||
|
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.
So is this
if in_end is not None:
self._in_scale = in_end - self._in_offset
else if out_end is not None:
self._in_scale = out_end - self._in_offset
else:
self._in_scale = 1.0
else: | ||
self._in_scale = in_end - self._in_offset | ||
|
||
if out_end is None: |
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.
can this also be rewritten to remove one layer of nesting?
if out_end is not None:
self._out_scale = out_end - self._out_offset
else if in_end is not None:
self._out_scale = in_end - self._out_offset
else:
self._out_scale = 1.0
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.
Took me a minute to realize how you just flipped the if
so that the else
comes first. Slow day. I did what you propose except that I kept the elif
checking for in_end is None
.
I was thinking that I can actually compute a single scale and offset parameter to minimize the math done in __call__
. What it your thought on that?
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 ❤️ contrapositives
I like elif in_end is not
'cause it keeps the formulas together and so parallel structure, but :shrugs: not gonna hold up a review over it.
not sure what you mean about single scale and offset...can you clarify?
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'm not really hung up on positives. Now that I've made the change, it does look better that way. Honestly, I am pretty sure I subconsciously avoid contrapositives after having worked in a place where they were discouraged by the coding standard for no really good reason.
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 can rewrite __call__
as return x * self._scale + self._offset
by adding probably about two extra lines to __init__
. Personally I think it's worth precomputing everything in __init__
, but I am OK with keeping the structure as-is if that is what you prefer.
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 no preference here, so go with whatever you think is more sensible/maintainable
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 will go ahead and make the change. I also just realized that when I removed the lines like self._in_offset = 0.0 if in_start is None else in_start
, I forgot to replace them with self._in_offset = in_start
and similar :)
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 there some sort of test that I can run on my example, like an image test?
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 made the additional change and ran the code this time to make sure it actually works as intended :)
|
||
fig = plt.figure() | ||
ax1 = fig.add_subplot(111) | ||
ax2 = fig.add_subplot(111, sharex=ax1, sharey=ax1, frameon=False) |
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 it necessary to have a shared axis to display this new ticker? That seems to be conflating two separate tasks.
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.
Not strictly necessary, but the conflation was deliberate. I wanted to be able to display both the original and the transformed values as well as to use this as a demo for #7528. There will be some modifications once that PR gets accepted (which I think is very likely once I find the time to write some additional tests for it).
lib/matplotlib/tests/test_ticker.py
Outdated
assert fmt.locs == locs | ||
assert fmt.formatter.locs == xlocs | ||
|
||
fmt.set_locs(None) |
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 is a teeny test, but it's a different one def test_clear_locs
or something
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.
Done
Tests included. Example code provided with some recipes from previous attempt.
7b6f083
to
822b01f
Compare
I don't think the travis error has anything to do with me... |
'nother bump. |
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.
OK, had a chance to look at this. It's mostly good except for the one example that I find a bit confusing.
""" | ||
Sets up the transformation such that `in_start` gets mapped to | ||
`out_start` and `in_end` gets mapped to `out_end`. The following | ||
shortcuts apply when only some of the inputs are specified: |
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'm sorry to say I was very confused by this description; it took me more than one reading to understand what it meant.
First, they're not really shortcuts, they're just rules. Second, the word 'input' is used for both arguments to the class __init__
in this sentence and for arguments to the instance's __call__
in each of the points below.
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.
After commenting on each of these elements, I think they may just be more confusing than it's worth. Is it really necessary to describe all the "shortcuts"? Would a mathematical equation and a set of defaults be more straightforward?
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 you are right. I wrote this when I was trying to figure out how I would want the different cases to work out mostly as notes for myself. Will fix.
`out_start` and `in_end` gets mapped to `out_end`. The following | ||
shortcuts apply when only some of the inputs are specified: | ||
|
||
- none: no-op |
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.
'none' should probably be 'no inputs specified'; I thought this was a typo for None
at first.
|
||
- none: no-op | ||
- in_start: translation to zero | ||
- out_start: translation from zero |
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.
For both of these, translation of what?
- in_start: translation to zero | ||
- out_start: translation from zero | ||
- in_end: scaling to one (divide input by in_end) | ||
- out_end: scaling from one (multiply input by in_end) |
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.
Should be out_end
?
- out_start: translation from zero | ||
- in_end: scaling to one (divide input by in_end) | ||
- out_end: scaling from one (multiply input by in_end) | ||
- in_start, out_start: translation |
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.
Translation of what?
lib/matplotlib/tests/test_ticker.py
Outdated
def transform2(self, x): | ||
return 2 * x | ||
|
||
@pytest.fixture() |
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.
You can drop the parentheses.
lib/matplotlib/tests/test_ticker.py
Outdated
|
||
def test_attributes(self, empty_fmt): | ||
# Using == is the right way to compare bound methods: | ||
# http://stackoverflow.com/q/41900639/2988730 |
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.
https
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.
Why? Both ways work fine. Is there some reason https is preferred?
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.
Just so we're clear, I'm going to make the change. I was just curious.
lib/matplotlib/tests/test_ticker.py
Outdated
|
||
@pytest.fixture() | ||
def fmt(self): | ||
fmt = self.empty_fmt() |
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 could take empty_fmt
as an argument to make it clear it uses the fixture as well.
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 did not know you could do that to non-test methods. Will definitely chain it that way.
lib/matplotlib/tests/test_ticker.py
Outdated
|
||
@pytest.fixture() | ||
def loc_fmt(self): | ||
fmt = self.fmt() |
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.
Same with fmt
here.
lib/matplotlib/ticker.py
Outdated
transformed values. | ||
|
||
The input may be an instance of | ||
:class:`matplotlib.ticker.Formatter` or an other callable with |
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.
an other -> another
Some small changes and rework of example transform code.
@QuLogic Based on your comments, I have greatly simplified the configuration of the linear transform in the example. I hope you like it. I found myself trying to figure out where I was going with the original version and just gave up :) I made all the other changes you recommended as well. |
@madphysicist Sorry this has completely fallen through the cracks (for 4 years!). Are you still interested in working on this? |
This had a lot of input, but I'll close for now as orphaned... |
MaxNLocator(integer=True)
.MaxNLocator(integer=True)
.x
andpos
as arguments, but actualFormatter
instances will get special treatment.Also, changed the annotation in PercentFormatter tests slightly (non-functional change).