ENH: Added share_tickers parameter to axes._AxesBase.twinx/y #7528
Conversation
|
@efiring. I made the parameter have to be explicitly specified by the user. The default is to share the ticker. Initially, even if the container is not shared, the actual Locator and Formatter are. It is then up to the savy user to mess things up for themselves. |
|
On 2016/11/28 12:53 PM, Joseph Fox-Rabinovitz wrote:
It is then up to the savy user to mess things up for themselves.
The problem is that when they do that, they tend to come back to us and
complain.
|
|
Given the way I structured it, the user has to first manually disable the |
009c32f to
a761873
Compare
|
@efiring Hopefully the explicit warnings I added in the documentation will be sufficient. Perhaps I should add an actual warning to the low-level functions that set up auto-scaling that will be triggered if |
Current coverage is 61.90% (diff: 100%)@@ master #7528 diff @@
==========================================
Files 109 173 +64
Lines 46780 56171 +9391
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 31075 34771 +3696
- Misses 15705 21400 +5695
Partials 0 0
|
54c9995 to
890c8b7
Compare
|
I think you push some commits for #7482 on here accidentally. |
|
@QuLogic. It was not entirely an accident. I wanted to make the documentation change in the example once (if) both branches get merged. That last commit should be in whichever branch gets merged last. My mistake was pushing it here early. |
|
Sorry, pushing all kinds of buttons unintentionally in mobile version. |
890c8b7 to
eb18ba8
Compare
|
@QuLogic. I undid the push. Once either PR gets accepted, I will add the example modifications to the other one. Thanks for the catch. |
|
While I realize that I need to add more tests for things like subplots using the new keyword, is the failure in coveralls something I should be worried about in general? |
|
@madphysicist do you have a screenshot of what this would look like (i.e. a shared axis with 2 distinct ticker formats)? I'm working on #7904 and wanted to be sure things would be consistent in terms of approach. |
|
@naoyak. Thanks for reaching out. I am glad to see you working on the auto-scaling, which I know next to nothing about. There is a screenshot of the expected behavior in a comment I made in the issue that started this PR. It also has the code used to generate it. The portion of the discussion following that comment also has some points by @efiring about autoscaling. I am not sure if they are pertinent to your PR though. |
9a51473 to
c75e624
Compare
|
Awesome, the screenshot helps. I don't think our PRs will clash in any way but if you are close to finishing this one up, I might just wait and rebase on top. |
|
I agree that there will likely be no clash. I was wondering if you have any thoughts about this comment. If the twinned axes have different locators and autoscaling is turned on, is there really a possibility for conflict between the locators? If so, how would you recommend resolving it? |
|
What do we mean here when we talk about a conflict in autoscaling? Do you mean that one of the twinned |
|
@naoyak Going off the comment I linked to in my previous post, there may be the possibility that the locator is used to determine bounds when autoscaling is turned on. Since my PR will allow different locators on each of the twinned axes, which one will determine the final bounds that are set (assuming that both have autoscaling on simultaneously, as they should)? I am not 100% convinced that there is a possibility for conflict at all, but I have not traced through the code enough or done any experiments to confirm one way or the other. I thought you might have more insight into the possibilities. |
lib/matplotlib/axis.py
Outdated
| @@ -604,6 +604,14 @@ class Ticker(object): | |||
| locator = None | |||
| formatter = None | |||
|
|
|||
| def __init__(self, ticker=None): | |||
There was a problem hiding this comment.
I have a slight preference to stay with the pattern the Artists use of an update_from method, but not enough to hold up the PR over.
There was a problem hiding this comment.
I have no problem doing it that way too. Functionally it won't make a difference what the name of the method is.
| self.xaxis.set_minor_locator(minl) | ||
| # Reset the formatter/locator. Axis handle gets marked as | ||
| # stale in previous line, no need to repeat. | ||
| if self._share_tickers: |
There was a problem hiding this comment.
I am confused, it looks like this behavior is what I thought it was, but the other behavior is what it used to do.
There was a problem hiding this comment.
The old way of doing things was:
- Copy the
Tickerobjects to the new axis (lines 973-4) - Save the formatters and locators (lines 979-982)
- Trash the formatters and locators with
_set_scale(line 985) - Restore the formatters and locators (lines 988-991)
This would restore for both the old and new axis since they always shared the same Ticker object.
I made this process a little more efficient:
- Set empty
Tickersfor the new axis (this one) (lines 993-4) - Let
_set_scalemess with the emptyTickers(line 1001) - Reset the
Tickers to either a reference (lines 1006-7) or a copy (lines 1009-10) of the old axis'sTickers.
Since _share_tickers defaults to True, the original behavior of having a reference to the original axis' Tickers is preserved unless explicitly asked for by the user. The key here is that _set_scale messes up the formatters and locators for its own purposes and it's effects have to be undone one way or another. I think my way is just simpler.
|
@tacaswell I switched |
|
Looks like everything is working. However, I would like to add a couple of tests with subplots and autoscaling before this gets merged. |
9b3b55b to
1f23212
Compare
Added copy constructor to axis.Ticker
202f91b to
546713e
Compare
Allow a copy of
Axis.minorandAxis.majorobjects to be made when axes are twinned. This will allow two twinned axes to have differentFormatterandLocatorobjects when they are both visible. The default of always sharing the same references (via a sharedmatplotlib.axis.Tickercontainer) has been preserved.I made the keyword parameter
share_tickerspropagate all the way toaxes._AxesBase.__init__rather than implementing the copy inaxes.AxesBase._make_twin_axes. The reason is that I would like to be able to modify the meaning ofsharexandshareydirectly in the constructor.Fixes #7376.
This PR includes tests.
A couple of places that I would ask reviewers to look at:
Axis.set_major/minor_formatter/locator. This would set theaxisattribute of the locators and formatters to the second axis, not the original one thattwinx/ywas called on in the first place. I have gone ahead and changed that behavior for both theshared_ticker=Truecase and theFalsecase. For theFalsecase this is pretty obvious, since we do not want to change theaxisattribute of the original formatter and locator at all. It should not cause any problems in theTruecase either, but it is technically a minor change in the functionality that I would like to bring attention to.share_tickersparameter correctly in axes/_base.py:473?Figure._make_key. I think that this is correct because the axes with and withoutshare_tickerswill serve different purposes visually, so it should be OK to treat them as different objects.axis.Tickerbecause it is really trivial. However, the code does get complete coverage in the tests that I wrote fortwinx/y.