Skip to content

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

Merged
merged 3 commits into from
Feb 11, 2019
Merged

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Aug 15, 2018

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 and ax.secondary_yaxis; here the work is all in _secondary_axes.py, and a new method in _axes.py.

Update 7 Sep:

Now called as

secax = ax.secondary_xaxis('top', conversion=(forward_func, inverse_func))

Please see the example and tests...

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)

@jklymak jklymak force-pushed the enh-secondary-axes branch from 6b6237c to 624b308 Compare August 15, 2018 23:01
@ImportanceOfBeingErnest
Copy link
Member

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

secax = ax.secondary_xaxis('top', conversion=lambda x: 2*np.pi/x)

Would this be possible, internally inverting any function numerically?

@jklymak
Copy link
Member Author

jklymak commented Aug 16, 2018

2 np.pi / x is already accounted for without transform (secondary_xaxis('top', conversion='inverted', otherargs=2* np.pi)). As are power laws. We could add “arbitrary” and have otherargs be two vectors like the above example. Then the user would be responsible for the numerics.

@jklymak
Copy link
Member Author

jklymak commented Aug 16, 2018

Arbitrary transform API:

Using an inverse as an example, though that is already supplied by secondary_xaxis('top', conversion='inverted', otherargs=2* np.pi):

functions

def 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)

Transforms

class 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))

Arbitrary

xin = 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)...

@jklymak
Copy link
Member Author

jklymak commented Aug 18, 2018

@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...

@ImportanceOfBeingErnest
Copy link
Member

I would vote for the functions. If I were to write this for my own purposes I'd do

ax.secondary_xaxis('top', conv=lambda x: .... , inv=lambda k: ...)

but if you think there is a reason to name the arguments differently, that's fine (otherargs sounds very general though).

@jklymak
Copy link
Member Author

jklymak commented Aug 18, 2018

@ImportanceOfBeingErnest I just didn't want there to be a plethora of kwargs, but maybe otherargs is too mysterious. I do need it for 'linear' and ='inverted' though...

@jklymak
Copy link
Member Author

jklymak commented Sep 2, 2018

@ImportanceOfBeingErnest, @efiring

  • added possibility of conversion=(forward, inverse) where these are forward and inverse functions
  • removed the "string" options since they are pretty easy to use as above.
  • removed 'otherargs' as a possible kwarg.

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:

sec = ax.secondary_xaxis('top', conversion=(lambda x: 3 / x, lambda x: 1 / (x * 3)))



def inverse(x):
return 1 / x

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

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?).

Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest left a 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.

@efiring
Copy link
Member

efiring commented Sep 2, 2018

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 conversions=(forward,inverse) or using 2 required keyword-only arguments. This simplifies everything: the mpl code base, the example, and the documentation, and as far as I can see, it imposes no significant extra work on the user under any circumstances. The resulting user code is more readable. There is no chance of getting the order of slope and intercept mixed up. Everyone is happier, right? Am I missing something? Is there any use case this doesn't handle cleanly? (I haven't spent time on this yet, so certainly I might be missing all sorts of things.)

@efiring
Copy link
Member

efiring commented Sep 2, 2018

Between the two options--single kwarg vs two kwargs--I am inclined to prefer the latter as the most readable.

self.apply_aspect()

# loop over self and child axes...
for ax in [self]:
Copy link
Member

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]))
Copy link
Member

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?

@jklymak
Copy link
Member Author

jklymak commented Sep 2, 2018

Two senior devs and @ImportanceOfBeingErnest for two-keyword, function-only interface

ax.secondary_xaxis('top', conversion=lamda x: 2/x, inverse=lambda x: 1 / (2 * x))

@timhoffm , @tacaswell? Anyone else object to this?

@ImportanceOfBeingErnest
Copy link
Member

As I understand it, @efiring prefers the two-argument notation conversion=lambda x: 2*x, inverse=lambda x: x/2. while I personnally prefer the single argument tuple conversion=(lambda x: 2*x, lambda x: x/2.).
I wouldn't completely object the other solution, but with the tuple notation it is clearer in my eyes that you always need such forwards and backwards function to come as pair, while two single arguments may not make it too clear that one needs to specify both to work.

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 twinx/twiny in order to simply format this scale differently or use a different locator. In such case specifying the secondary_*axis as e.g. ax2 = ax.secondary_yaxis('right', conversion=lamda x: x, inverse=lambda x: x) instead of ax2 = ax.secondary_yaxis('right') feels quite cumbersome.

@efiring
Copy link
Member

efiring commented Sep 2, 2018

The no-conversion case can be handled concisely by making it the default, conversion=None, inverse=None.

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, conversion=(reciprocal, reciprocal), with a default of conversion=None, but I think the two-kw form is a bit more readable and easier to document. I am adamant that we need to choose one or the other.

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.

@ImportanceOfBeingErnest
Copy link
Member

I think my point was that a signature which looks like

 function(location, conversion=None, inverse=None)

does not make it clear that you need to specify conversion and inverse simultaneously, while with

function(location, conversion, inverse)

that would be obvious. However if deciding for that latter case, function(location), meaning "no conversion" would be impossible.

A signature like

function(location, conversion=None)

would overcome this, allowing for "no conversion" as None or a custom conversion in form of a tuple.

If we expect this feature to be widely used, I'm pretty sure that sooner or later people would demand for conversion also taking a constant, or a slope + offset, essentially what the current implementation already allows for. So it may be beneficial to implement this now and do it the way it can be agreed upon than having a user trying to implement it later and going through all the details of implementation in their PR when it comes in.

@ImportanceOfBeingErnest
Copy link
Member

Oh, I think I forgot to say that I would certainly agree that one would not want to have a mixture like conversion=(forward,backward), inverse=None - that would sure be confusing and hard to document.

@efiring
Copy link
Member

efiring commented Sep 2, 2018

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.

@ImportanceOfBeingErnest
Copy link
Member

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%.


fig, ax = plt.subplots()
ax.loglog(range(1, 360, 5), range(1, 360, 5))
ax.set_xlabel('wavenumber [cpkm]')
Copy link
Contributor

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)")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure...

@jklymak jklymak force-pushed the enh-secondary-axes branch 2 times, most recently from 6cfe2ca to c532b71 Compare February 5, 2019 01:47
Copy link
Member

@efiring efiring left a 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:
Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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.

Copy link
Member Author

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)
Copy link
Member

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
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Contributor

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.

Examples
--------

Add a secondary axes that shows both wavelength for the main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete "both".

Copy link
Member Author

@jklymak jklymak Feb 5, 2019

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:

Copy link
Member

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?

Copy link
Member Author

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]')


Copy link
Member

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.

Copy link
Member Author

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


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,
Copy link
Member

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.

def set_zscale(self, value, **kwargs):
"""
Set the scaling of the z-axis: %(scale)s
Set the x-axis scale.
Copy link
Member

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?

Copy link
Member Author

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.
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member Author

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.

@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 5, 2019
@jklymak
Copy link
Member Author

jklymak commented Feb 8, 2019

ping @anntzer @efiring I think I hit all your requests...

@efiring
Copy link
Member

efiring commented Feb 8, 2019

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

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).

Copy link
Member Author

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lamda -> lambda.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dot

class FuncScaleLog(LogScale):
"""
Provide an arbitrary scale with user-supplied function for the axis and
then put on a logarithmic axes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dot

@anntzer anntzer dismissed their stale review February 8, 2019 12:53

original review comments handled

@jklymak jklymak force-pushed the enh-secondary-axes branch from 1b92125 to d5c2400 Compare February 8, 2019 15:07
@jklymak jklymak force-pushed the enh-secondary-axes branch from d5c2400 to 85c1b99 Compare February 8, 2019 15:08
@jklymak
Copy link
Member Author

jklymak commented Feb 11, 2019

... merge?

@efiring efiring merged commit df9e346 into matplotlib:master Feb 11, 2019
@ImportanceOfBeingErnest
Copy link
Member

I just ran the first example from this PR and the result looks like this on screen:

image

If resizing the figure window with the mouse this can become as bad as

image

The saved figure is fine though.
(This is with master fetched a few minutes ago, python 3.6.6 on windows 8.1, Qt5Agg)

Let me know if you need further details or want me to test anything particular with my configuration.

@jklymak
Copy link
Member Author

jklymak commented Feb 12, 2019

Well, huh - never tried resizing the window!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: ticks axis labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants