-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: add secondary x/y axis #11859
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
ENH: add secondary x/y axis #11859
Conversation
d921e18
to
6b6237c
Compare
6b6237c
to
624b308
Compare
The creation of a secondary axes with a non-linear transform seems quite complicated. I'm wondering if there is an easier way to achieve this? Ideally one would probably desire something like
Would this be possible, internally inverting any function numerically? |
|
Arbitrary transform API:Using an inverse as an example, though that is already supplied by functionsdef forward_func(x):
return 2 * np.pi / x
def inverse_func(x):
return x / (2 * np.pi)
secondary_xaxis('top', conversion=forward_func, otherargs=inverse_func) Transformsclass LocalInvert(Transform):
input_dims = 1
output_dims = 1
is_separable = True
has_inverse = True
def __init__(self, fac):
Transform.__init__(self)
self._fac = fac
def transform_non_affine(self, values):
return self.fac / values
def inverted(self):
""" we are just our own inverse """
return LocalInvert(1.0 / self.fac)
secondary_xaxis('top', conversion=LocalInvert(2 * np.pi)) Arbitraryxin = np.arange(300)
xout = 2 * np.pi / xin
secondary_xaxis('top', conversion='arbitrary', otherargs=(xin, xout)) (Note this has pretty bad numerical issues under zooming (will collapse to linear for delta xin < 1, and will be linear extrapolation for xin < 0 or xin > 300)... |
@ImportanceOfBeingErnest and others, which of the three is the better arbitrary interface? I guess I like the functions approach the best (not implemented yet). Its basically the same as the Transform, but with a bit less overhead. Internally, it would just be a transform. Of course we should still support a Transform... |
I would vote for the functions. If I were to write this for my own purposes I'd do
but if you think there is a reason to name the arguments differently, that's fine ( |
@ImportanceOfBeingErnest I just didn't want there to be a plethora of kwargs, but maybe |
624b308
to
3a25b84
Compare
@ImportanceOfBeingErnest, @efiring
I could easily put the string options back, but I think it makes the docs a lot cleaner, and is more powerful for the user just to supply the function. It works very easily with lambda functions:
|
|
||
|
||
def inverse(x): | ||
return 1 / x |
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.
It might be a bit confusing that the example uses the one and only case where forward and inverse are actually the same, although it probably makes sense to you a/x
to show how a log scale behaves. Would something like 2./np.sqrt(x)
also work here?
plt.show() | ||
|
||
########################################################################### | ||
# The conversion can be a linear slope and an offset as a 2-tuple. It can |
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.
Would it make sense to explicitely show the use of this (slope,offset)? I think this is somehow the most common case, or at least the one where there is already a Celsius->Fahrenheit example in the gallery (...which could then be replaced?).
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 conversion=(forward,inverse)
is really useful. 👍
The examples could actually be made much more vivid and explicit, using real world applications like Celsius-Fahrenheit, Pascal-Torr, wavelength-wavevector. But that could also be done independently of this PR.
I would prefer simplifying the API further: accept only a pair of functions, and allow only one of the obvious ways of specifying them: as |
Between the two options--single kwarg vs two kwargs--I am inclined to prefer the latter as the most readable. |
lib/matplotlib/axes/_base.py
Outdated
self.apply_aspect() | ||
|
||
# loop over self and child axes... | ||
for ax in [self]: |
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 doesn't look like it is looping over anything other than self.
else: | ||
warnings.warn("location must be '{}', '{}', or a " | ||
"float, not '{}'.".format(location, | ||
self._locstrings[0], self._locstrings[1])) |
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.
It looks like you have the format
arguments out of order; location
should be last, not first, shouldn't it?
Two senior devs and @ImportanceOfBeingErnest for two-keyword, function-only interface
@timhoffm , @tacaswell? Anyone else object to this? |
As I understand it, @efiring prefers the two-argument notation I would not abbandon at least the single factor solution for the following reason: I think part of the motivation for the secondary axes was to get a better way of creating an identical scale on the other side of the plot without abusing |
The no-conversion case can be handled concisely by making it the default, In documentation and examples, I suggest avoiding the mystical-looking lambda, and instead define named functions as needed: def reciprocal(x):
return 1/x
ax2 = ax.secondary_yaxis('right', conversion=reciprocal, inverse=reciprocal) I'm not adamantly opposed to the tuple alternative, This feature--the axis with a different but functionally-related scale--is a wonderful addition to mpl, and one that even the newest mpl user and the least programming-oriented user should be comfortable with. It is the kind of API for which we should put heavy weight on simplicity, explicitness, and readability. |
I think my point was that a signature which looks like
does not make it clear that you need to specify
that would be obvious. However if deciding for that latter case, A signature like
would overcome this, allowing for "no conversion" as If we expect this feature to be widely used, I'm pretty sure that sooner or later people would demand for |
Oh, I think I forgot to say that I would certainly agree that one would not want to have a mixture like |
The problem with giving in to demand (if it were to arise) for multiple input forms is that it makes everything more complicated, for what I think is negligible gain in convenience. I'm really worried about code bloat in mpl, and excessive complexity in API and in documentation. |
To me it looks like not allowing for the constant factor or slope + offset would save you some 20 lines of the 580 lines long code (from line 248 to 271), which is 4%. |
lib/matplotlib/axes/_axes.py
Outdated
|
||
fig, ax = plt.subplots() | ||
ax.loglog(range(1, 360, 5), range(1, 360, 5)) | ||
ax.set_xlabel('wavenumber [cpkm]') |
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.
cpkm looks a bit domain-specific? perhaps "/km" (or "period (s)"/"frequency (Hz)")
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.
Sure...
6cfe2ca
to
c532b71
Compare
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.
Looks good; I hadn't realized how much work would be required for this functionality. My comments are quite minor.
axes with only one axis visible via `.Axes.axes.secondary_xaxis` and | ||
`.Axes.axes.secondary_yaxis`. | ||
|
||
If we want to label the top of the axes: |
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 suggest eliminating this first part of the example, since it really is not what the new functionality is for, and as illustrated here it duplicates existing functionality. Instead, go straight to the motivating use case, so that will be what people will see first, without distraction.
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
plt.show() | ||
|
||
########################################################################### | ||
# However, its often useful to label the secondary axis with 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.
Having eliminated the example above, this comment section can be reduced to something like its last sentence.
|
||
|
||
def rad2deg(x): | ||
return x * 180 / np.pi |
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.
Also in the interest of compactness and getting to the point, you could eliminate the function definitions and use np.deg2rad
and np.rad2deg
.
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.
Well, I wanted to have these as templates to the user. Otherwise, I find it a bit mysterious.
|
||
xold = np.arange(0, 11, 0.2) | ||
# fake data set relating x co-ordinate to another data-derived co-ordinate. | ||
xnew = np.sort(10 * np.exp(-xold / 4) + np.random.randn(len(xold)) / 3) |
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.
Perhaps it is obvious, but you might add, "Note: we must ensure that both coordinates are monotonic."
""" | ||
Add a second x-axis to this axes. | ||
|
||
For example if we want to have a second scale for the data plotted on |
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 eliminate this sentence and go straight to the example.
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.
Did you intend to do this, but forget?
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.
Ooops, I somehow screwed up a transfer between machines.
OTOH, not sure about this one. Note there is a long docstring interp between here and the example, so maybe its good to have the extra line?
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 agree that the entra sentence doesn't add much.
lib/matplotlib/axes/_axes.py
Outdated
Examples | ||
-------- | ||
|
||
Add a secondary axes that shows both wavelength for the main |
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.
Delete "both".
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.
Fixed to read:
The main axis shows frequency, and the secondary axis shows period:
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.
But I don't see this change on github; am I using github incorrectly, or did you not push the change?
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.
Sorry about that. Fixed now...
ax.loglog(range(1, 360, 5), range(1, 360, 5)) | ||
ax.set_xlabel('frequency [Hz]') | ||
|
||
|
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.
Out of curiosity: does the matplotlib plot directive require 2 blank lines before functions? It looks like you are using this spacing consistently.
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.
flake8 requires it for the actual examples, but not here I don;'t think... Fixed
lib/matplotlib/scale.py
Outdated
|
||
functions : (callable, callable) | ||
two-tuple of the forward and inverse functions for the scale. | ||
The forward function must have an inverse and, for best behavior, |
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.
Maybe just say it must be monotonic; actually, if it is not monotonic, it doesn't have an inverse function.
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
def set_zscale(self, value, **kwargs): | ||
""" | ||
Set the scaling of the z-axis: %(scale)s | ||
Set the x-axis scale. |
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.
Don't you mean the z-axis? And aren't the changes in this file completely unrelated to this 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.
Goops, sorry, you are right.
This was necessary because of the doctoring interpolation - It led to two Parameters
sections in this docstring, and doc build failure.
""" | ||
See `.secondary_xaxis` and `.secondary_yaxis` for the doc string. | ||
While there is no need for this to be private, it should really be | ||
called by those higher level functions. |
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.
Then maybe it should at least start out as private. Is there any advantage to making it public at this point?
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.
Well, there are some methods the user may want to manipulate?
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.
Plus this is the returned object, so users may want to see the documentation for it.
I see a total of only 2 commits, the original, and one from a couple days ago which has only some of the indicated changes in it. Either something is going wrong with the way I am interpreting or using this web page, or you didn't push something you thought you pushed. |
|
||
|
||
_secax_docstring = ''' | ||
Warnings |
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.
warning could use a rst directive, as in https://numpydoc.readthedocs.io/en/latest/format.html#other-points-to-keep-in-mind.
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.
Huh, the docstrings aren't rendering the interpolation at all...
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 not sure why this should be an rst directive versus the numpy doc heading. Is there a difference?
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.
Looking at numpydoc's implementation they just internally translate warnings sections to .. warnings:
, but also put the sections in their own (consistent) order (as seen in the rendered docs); I mildly prefer the latter as you can have them wherever you want or nested under other constructs (e.g. a bullet list) but it doesn't matter too much.
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.
ummm, so which do you prefer? I dont' really care - easier to remember the numpydoc way than the rst way
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.
Strictly personally I prefer the rst directive, but again anything works here (though you will need to use the directive if you want the warning to show up above the parameters in the rendered html).
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, I'll leave as is - I can see some argument for the warning at the top, but don't think its a big deal.
|
||
If a 2-tuple of functions, the user specifies the transform | ||
function and its inverse. i.e. | ||
`functions=(lamda x: 2 / x, lambda x: 2 / x)` would be an |
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.
lamda -> lambda.
lib/matplotlib/scale.py
Outdated
@@ -163,8 +163,7 @@ def __init__(self, axis, functions): | |||
|
|||
functions : (callable, callable) | |||
two-tuple of the forward and inverse functions for the scale. | |||
The forward function must have an inverse and, for best behavior, | |||
be monotonic. | |||
The forward function must be monotonic |
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.
dot
lib/matplotlib/scale.py
Outdated
class FuncScaleLog(LogScale): | ||
""" | ||
Provide an arbitrary scale with user-supplied function for the axis and | ||
then put on a logarithmic axes |
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.
dot
1b92125
to
d5c2400
Compare
d5c2400
to
85c1b99
Compare
... merge? |
I just ran the first example from this PR and the result looks like this on screen: If resizing the figure window with the mouse this can become as bad as The saved figure is fine though. Let me know if you need further details or want me to test anything particular with my configuration. |
Well, huh - never tried resizing the window! |
PR Summary
(This is #11589, but I closed that PR and force-pushed so I can't re-open; ping @ImportanceOfBeingErnest who had commented and @efiring who promised to take a look)
New methods
ax.secondary_xaxis
andax.secondary_yaxis
; here the work is all in_secondary_axes.py
, and a new method in_axes.py
.Update 7 Sep:
Now called as
Please see the example and tests...
PR Checklist