Skip to content
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

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

Merged
merged 4 commits into from Dec 17, 2012

Conversation

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

pelson commented Dec 4, 2012

Looks good to me.

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

Cheers,

@WeatherGod
Copy link
Member Author

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

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

@WeatherGod
Copy link
Member Author

No, the one I asked about on the dev mailing list a couple hours ago.
@mdboom thinks it has to do with using deprecated numpy api. I am going to
try rebuilding later today with numpy 1.7.

@dmcdougall
Copy link
Member

Whew ok. That's a relief.

@mdboom
Copy link
Member

mdboom commented Dec 4, 2012

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

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

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

efiring commented Dec 4, 2012

On 2012/12/04 8:41 AM, Benjamin Root wrote:

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

Whatever you do, make sure spy is setting the same tight limits as at
present. I really don't see any point in changing it; it is a very
special-purpose function, and it isn't broken. To me, it is logical
that it should work the way it does, turning off autoscaling by
explicitly setting its limits.

@WeatherGod
Copy link
Member Author

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

@dmcdougall
Copy link
Member

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

@WeatherGod
Copy link
Member Author

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

@dmcdougall
Copy link
Member

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

@WeatherGod
Copy link
Member Author

no, go ahead and merge.

dmcdougall added a commit that referenced this pull request Dec 17, 2012
inverting an axis shouldn't affect the autoscaling setting
@dmcdougall dmcdougall merged commit 07fa831 into matplotlib:v1.2.x Dec 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants