inverting an axis shouldn't affect the autoscaling setting #1557

Merged
merged 4 commits into from Dec 17, 2012

6 participants

@WeatherGod
Matplotlib Developers member

invert_xaxis() and friends shouldn't blindly set the axis limits. The autoscaling state should be left unchanged. There may be other places in the codebase where this change should be made.

Closes #1553

@pelson
Matplotlib Developers member

Looks good to me.

Is this easy to test? If so, would you mind adding a simple test for this?

Cheers,

@WeatherGod
Matplotlib Developers member

Yes, I was just about to push up a commit that tests this, but then I ran into the build failure on my machine.

@dmcdougall
Matplotlib Developers member

@WeatherGod "The" build failure? The ones we've been seeing recently on Travis? The travis builds are succeeding now...

@WeatherGod
Matplotlib Developers member
@dmcdougall
Matplotlib Developers member

Whew ok. That's a relief.

@mdboom
Matplotlib Developers member

As for "other places in the code that may need this update": I just did a quick grep. hist2d and spy both set the limits unconditionally like this -- I wonder what the implications of adding those are.

@WeatherGod
Matplotlib Developers member

Ok, looks like I accidentally picked up a couple of the cherry-picked commits in my last rebase. Hopefully that isn't too much of an issue.

I will look into spy and hist2d.

@WeatherGod
Matplotlib Developers member

wrt spy() and hist2d(), I have to question why they even need to explicitly set the limits. Maybe at the time, the autoscaling logic wasn't very good, but it is much more mature now. Perhaps we should update these to use autoscale_view()?

@efiring
Matplotlib Developers member
@WeatherGod
Matplotlib Developers member

We can certainly still do the same setting of the limits. I just suggest that the autoscaling state should be left alone.

@dmcdougall
Matplotlib Developers member

@WeatherGod Is this good to go or are you going to add hist2d and spy to this PR?

@WeatherGod
Matplotlib Developers member

I think I will leave spy and hist2d out. We will have to ponder it some more.

@dmcdougall
Matplotlib Developers member

@WeatherGod Ok. Are you needing to add anything or can I merge this?

@WeatherGod
Matplotlib Developers member

no, go ahead and merge.

@dmcdougall dmcdougall merged commit 07fa831 into matplotlib:v1.2.x Dec 17, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment