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

Refactors axis3d.py to address issue #3610 #3787

Merged
merged 10 commits into from Feb 28, 2015

Conversation

jbbrokaw
Copy link
Contributor

No description provided.

@tacaswell tacaswell added this to the v1.5.x milestone Nov 13, 2014
@tacaswell
Copy link
Member

Can you add a test (probably png only) showing that this works?

attn @WeatherGod

@WeatherGod
Copy link
Member

Heh, I feel silly, not realizing how low-hanging it was. Probably need a "what's new" entry stating that labelpad now works.

My remaining concern is units. I don't know what they are in regular plots, and I am fairly sure they aren't the same as this in mplot3d. Also, is there an existing rcparam that we might need to start respecting?

@jbbrokaw
Copy link
Contributor Author

I added a little test; I haven't contributed here before so let me know if it's not what you're looking for.

The units are "deltas" in move_from_center, which seem to be (max - min)/12 for each dimension, definitely not points like for regular plots. I used the default of 1.6 that was there earlier, which overrides the value set in matplotlib.axis.Axis, which gets it from rcParams['axes.labelpad'] (which defaults to 5).

@WeatherGod
Copy link
Member

You will need to move that test over to the tests that are in mpl_toolkits. Also, don't forget to include the baseline images.

As for the points versus delta issue, I am conflicted. As far as I understand, units of points are completely meaningless in 3d projections. @mdboom, what do you think? Ideally, I would like to get mplot3d to behave more and more like regular axes (with all of the nutty goodness of rcparams and stylesheets), but that does mean either generalizing some of the existing 2d concepts, or figuring out a way to kludge some sort of 2d <=> 3d conceptual mapping.

@jbbrokaw
Copy link
Contributor Author

I added a simple map from the 2D labelpads to the 3D ones --- basically the 3D version is 0.05 * labelpad2D + 1.35. This is only sort of OK, but it stops overriding the values inherited from the 2D versions.
These numbers could probably be improved; I got them by just playing around with 2D & 3D plots.

@tacaswell
Copy link
Member

This should get an entry in whats_new to advertise the new feature.

@WeatherGod
Copy link
Member

I would be careful with such a function. It would be highly dependent upon
the figure size. In fact, isn't a point defined a 1/72 of an inch or
something?

On Mon, Nov 17, 2014 at 6:39 PM, Thomas A Caswell notifications@github.com
wrote:

This should get an entry in whats_new to advertise the new feature.


Reply to this email directly or view it on GitHub
#3787 (comment)
.

@tacaswell
Copy link
Member

@WeatherGod Can you be responsible for merging this when you are happy?

@WeatherGod
Copy link
Member

Sure thing

On Mon, Nov 17, 2014 at 9:45 PM, Thomas A Caswell notifications@github.com
wrote:

@WeatherGod https://github.com/WeatherGod Can you be responsible for
merging this when you are happy?


Reply to this email directly or view it on GitHub
#3787 (comment)
.

@jbbrokaw
Copy link
Contributor Author

@WeatherGod commit 7a7e317 is mostly an experiment in making label padding less dependent on figure size (i.e., more like in 2D plots). It's a bit kludgey (and calling its units "points" is certainly misleading), but its main problem is that the tick label padding does currently scale with figure size, so the tick labels and axis labels tend to overlap a lot as the figure size changes. The tick label paddings could also be made to act more like they were in units of points if this is what we want (the tick label padding is also currently set in the _AXINFO rather than from rcParams). On the other hand, it does not look too bad if the tick label and axis label paddings scale together. What is your opinion?

@tacaswell
Copy link
Member

This breaks the tests for mpl3D. I hope this is just a matter of re-setting the defaults.

@WeatherGod
Copy link
Member

I think we are getting very close. I want the units to be equivalent, but I
doubt we would be able to truly compute the distances until I rework the
projection system, which is not coming along very well... emulation with
proper documentation may have to be sufficient for now.

It makes sense to get the tick label padding out as well. Let's go for a
two-fer!

On Wed, Nov 26, 2014 at 11:13 PM, Thomas A Caswell <notifications@github.com

wrote:

This breaks the tests for mpl3D. I hope this is just a matter of
re-setting the defaults.


Reply to this email directly or view it on GitHub
#3787 (comment)
.

@jbbrokaw
Copy link
Contributor Author

jbbrokaw commented Dec 8, 2014

I extended the approach to tick labels, which basically works -- they should respect tick.pad from the 2D code now.
However, I'm pretty sure the test failures are a fundamental problem with my approach. I'm using axes.bbox.height and axes.bbox.width to estimate the plot size, but this seems to be very different for pdf rendering than for the other renderers. This is causing the padding size estimation to be too large for pdfs only, and I haven't been able to compensate. Is there a more consistent way to get the size of the plot in actual distance (points or inches)?

@tacaswell
Copy link
Member

fig.get_size_inches() returns the size of the figure in inches and you can use the transform stack to get convert the axes bounding box to which ever coordinate system you want.

iirc, pdf has dpi hard coded to 72?

@jbbrokaw
Copy link
Contributor Author

jbbrokaw commented Dec 9, 2014

Thanks, getting the figure size in inches and scaling from there fixed it -- pdfs are behaving consistently now.

@tacaswell tacaswell modified the milestones: proposed next point release, next point release Feb 19, 2015
@WeatherGod
Copy link
Member

sorry for having this fall off the table. I think this is very good. Let me do a couple last tests before I merge this in.

@WeatherGod
Copy link
Member

Yup, interactively, it looks good to me. I was able to resize the figure window to weird shapes, and the labels remained the same distance away from the axis, and spinning around the cube doesn't seem to mess anything up. Great job!

WeatherGod added a commit that referenced this pull request Feb 28, 2015
@WeatherGod WeatherGod merged commit 36b0f23 into matplotlib:master Feb 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants