Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix formatter reset when twin{x,y}() is called #1145

Merged
merged 4 commits into from

6 participants

@dmcdougall
Collaborator

This addresses #1110. In particular.

The functions Axes.twiny and Axes.twinx create a new set of Axes objects. The Axes.cla method is called during the constructor to do some setup. If a shared axes is passed in, a copy of the scale is executed, which overwrites the current formatter. To fix it, I've stored in a temporary variable before the scale copy, then restore it to use the original formatter.

@pelson
Collaborator

Damon - awesome! I thought this was going to be a mammoth job to fix this problem, so its awesome that it turned out to be such a simple change.

I wonder if it would be better to add a kwarg to set_scale which disables the default scale setting instead? This would have the benefit of preserving the 2 formatters, and the 2 locators, and mean that you don't have to save the original and then re-apply it post calling set_scale. Would there be any drawbacks (apart from adding a new keyword) to this approach?

@dmcdougall
Collaborator

Thanks Phil. Like I said, I've got some free time. Though this took a quite a bit of digging to track down!

So, I was wondering almost the same thing. After I made this PR I thought perhaps the problem is not cla but in the set_scale method itself, which sets the locater and formatter to the default. I then realised it would take a considerable amount of effort to adapt it such that it checks if a current locater/formatter exists and, if so, applies. Otherwise use the default. I hadn't though of utilising a kwarg... that would be a lot easier and perhaps simpler, though that depends heavily on your definition of 'disable the default scale setting'.

Perhaps it's actually better to separate the scale, formatter and locaters more. Keep them more modular. Currently set_scale fiddles with formatters and locaters. In my opinion, that doesn't 'do what it says on the tin'.

Thoughts?

@pelson
Collaborator

Currently set_scale fiddles with formatters and locaters. In my opinion, that doesn't 'do what it says on the tin'.

I'm inclined to agree. However, that would make a significant change which could subtly change existing code's behaviour. Given that we have now missed the 1.2.x feature freeze deadline, the only "proper" way of getting this useful fix into 1.2 is if the change is more of a minor bug-fix (such as the one you have proposed, or by adding a simple keyword to trigger the behaviour). My preference would be the latter, but either way is better than not at all.

Does anyone else have a preference?

@dmcdougall
Collaborator

I wanted to add one more thing. After a little more thought, I think adding a kwarg is not the right option. Just by using the same train of thought as above, it adds functionality to {s,g}et_scale that shouldn't be there. Also, in the future when {s,g}et_scale is updated to not fiddle with the locaters and formatters, it will need to be removed anyway.

@mdboom
Owner

I think the fact that set_scale updates the formatters and tickers is convenient for a lot of cases where the user wants to set the scale and have the formatters and tickers use the obvious defaults for that scale. However, I agree with @pelson, that that doesn't "do what it says on the tin". I think the real way forward is to have both a way to set only the scale and a way to set everything in tandem.

But as for this quick fix for an obvious bug, I think this does the trick.

@pelson
Collaborator

But as for this quick fix for an obvious bug, I think this does the trick.

OK. My preference remains with a toggle keyword, but I agree that this approach "does the trick", so lets run with it.

Presumably there isn't a test for this, which we may wish to add. Additionally, we will want to document the bug fix. (there are 3 places to put change logs, and I am never entirely confident where to put things).

@WeatherGod
Collaborator
@mdboom
Owner

I think this particular one should probably go in api_changes.rst -- something like "calling twinx/twiny will no longer override the current formatter".

@efiring
Owner
@dmcdougall dmcdougall Update update API changes
Explicitly document the slight change in behaviour of Axes.twin[xy]
62dc662
@travisbot

This pull request fails (merged 62dc662 into 94c53e1).

lib/matplotlib/axes.py
@@ -852,7 +852,15 @@ def cla(self):
self.xaxis.minor = self._sharex.xaxis.minor
x0, x1 = self._sharex.get_xlim()
self.set_xlim(x0, x1, emit=False, auto=None)
+
+ # Save the current formatter so we don't lose it
+ frmt = self._sharex.xaxis.get_major_formatter()
@pelson Collaborator
pelson added a note

This doesn't actually preserve the locators nor the minor formatters, unless I am mistaken. It was for this reason that I proposed the kwarg addition, to avoid you having to copy and paste the four line prefix and suffix to set_scale. Given that we seem to have agreed to not implementing the keyword, I still think you will need to handle the minor formatter, and both locators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson
Collaborator

This will need a simple test before merging. Something that exercises both locators and formatters using both the major and minor positions.

@travisbot

This pull request fails (merged 6a2d1e9 into 94c53e1).

@pelson pelson commented on the diff
lib/matplotlib/tests/test_axes.py
@@ -62,6 +62,29 @@ def test_formatter_large_small():
y = [500000001, 500000002]
ax.plot(x, y)
+@image_comparison(baseline_images=["twin_axis_locaters_formatters"])
@pelson Collaborator
pelson added a note

We probably only need to test this with one format. Perhaps png is best performance wise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson
Collaborator

Thanks Damon. All looks good to me. The only comment might be to reduce the number of file formats being tested. TBH, in this case it might not have even been necessary to produce a visual test, but I don't propose that you change that now.

Thanks for this.

@mdboom mdboom merged commit 6a2d1e9 into matplotlib:master

1 check failed

Details default The Travis build failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 25, 2012
  1. @dmcdougall
Commits on Aug 30, 2012
  1. @dmcdougall

    Update update API changes

    dmcdougall authored
    Explicitly document the slight change in behaviour of Axes.twin[xy]
Commits on Sep 1, 2012
  1. @dmcdougall
  2. @dmcdougall

    Add a twin axes test

    dmcdougall authored
Something went wrong with that request. Please try again.