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

[dmcdougall]

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]

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]

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]

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]

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]

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]

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]

If it is a new feature, whats_new.rst
If it is a modification to existing call signatures, behavior that may
impact existing user code, api_changes.rst
The use of CHANGELOG, I think, is slowly diminishing as it is more a
vestige of the subversion days, IMHO. At this point, I would use that only
for major fundamental changes/additions/refactoring.

Admittedly, there are times when changes seem to fall into both categories
of whats_new.rst and api_changes.rst. When in doubt, put it in both
places. Another category of changes that would go into whats_new.rst is if
a long-outstanding, highly visible bug was fixed. For example, suppose
that we fixed a major performance issue or memory leak, we would want to
advertise this fact. Another would be allowing for multiple calls to
show(), or the normalization of behavior across backends back in v1.0.1.
These are things that we would want to advertise in a "what's new" section.

[mdboom]

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]

On 2012/08/29 3:54 AM, Michael Droettboom wrote:

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

Agreed. It is a fix for a somewhat obscure bug, not a substantially new
feature.

[travisbot]

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

[pelson]

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.

[pelson]

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]

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

[pelson]

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.