Fix mplot3d projection #8896
Fix mplot3d projection #8896
Conversation
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
| @@ -985,6 +999,8 @@ def get_proj(self): | |||
| point. | |||
|
|
|||
| """ | |||
| pb_aspect = np.array([1, 1, 0.75]) | |||
There was a problem hiding this comment.
Strictly speaking, this isn't just the aspect, but also the scaling. Is there already scaling in place somewhere else? Seems too convenient that everything just magically fits into the viewing area.
9355ac4 to
5922edd
Compare
|
Plot is now scaled to sort-of look like it did before, in the default view. Note that because the plot is now actually stretched horizontally, but the camera is not, the view angle looks different. Options:
|
|
Reasonably straightforward to implement axis=equal from here too |
| # in the superclass, we would go through and actually deal with axis | ||
| # scales and box/datalim. Those are all irrelevant - all we need to do | ||
| # is make sure our coordinate system is square. | ||
| figW, figH = self.get_figure().get_size_inches() |
There was a problem hiding this comment.
Why use the figure's aspect ratio? What if the figure has 3 subplots all in a row?
There was a problem hiding this comment.
Because all sizes in matplotlib are relative to the figure. pb.shrunk_to_aspect seems to take the ratio of the top-level parent (the figure), and the desired ratio. This is because the end result is the size relative to the parent, as all objects in matplotlib seem to be sized - so to ensure a given aspect ratio, you need to account for that of the parent
Note that this is the same as the normal aspect=equal code, so is presumably correct.
There was a problem hiding this comment.
Ping @efiring, as I think you are the resident expert on all the ways this could possibly go wrong.
There was a problem hiding this comment.
I think it makes sense. I have no intention of spending the time required to understand mplot3d well enough to make a detailed review, but in the absence of any such review I am inclined to trust that this PR is a major improvement, as it appears to be fixing a basic design flaw.
|
I wouldn't really know. I think @efiring is the expert on these things.
…On Wed, Aug 2, 2017 at 4:53 PM, Eric Wieser ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/mpl_toolkits/mplot3d/axes3d.py
<#8896 (comment)>
:
> @@ -247,6 +247,20 @@ def tunit_edges(self, vals=None, M=None):
(tc[7], tc[4])]
return edges
+ def apply_aspect(self, position=None):
+ if position is None:
+ position = self.get_position(original=True)
+
+ # in the superclass, we would go through and actually deal with axis
+ # scales and box/datalim. Those are all irrelevant - all we need to do
+ # is make sure our coordinate system is square.
+ figW, figH = self.get_figure().get_size_inches()
Because all sizes in matplotlib are relative to the figure.
pb.shrunk_to_aspect seems to take the ratio of the top-level parent (the
figure), and the desired ratio.
Note that this is the same as the normal aspect=equal code, so is
presumably correct.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#8896 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-E32123ChZda_Y-gPCtOSEXUWFvjks5sUOHjgaJpZM4OZVo9>
.
|
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
| return 'azimuth=%d deg, elevation=%d deg ' % (self.azim, self.elev) | ||
| # ignore xd and yd and display angles instead | ||
| return 'azimuth={:.0f} deg, elevation={:.0f} deg '.format( | ||
| self.azim, self.elev) |
There was a problem hiding this comment.
Bug fixed here on fractional view angles
There was a problem hiding this comment.
Extracted to #12573. I misremembered - the crash was only for non-finite values.
|
Any progress here? |
|
Unfortunately not. Progress would be noted here. |
|
I was under the impression the ball was not in my court here. Rebasing shouldn't be too awful, but I don't see much point without further review. |
|
@eric-wieser Not an expert in mpl3d and thus I have no idea who would have to take up the torch. I was just stating that the thread still reflects the current status. |
|
@timhoffm: I'll rebase for good measure, it'll allow someone to work from this branch at least if they need cubic plots. Can you remind me how to regenerate the baseline_images? Is there a mechanism to say "assume the new image is correct for all failed image comparison tests"? |
There was a problem hiding this comment.
In the interest of getting things moving, I will approve of this subject to the necessary rebase and addition of "what's new" and "API changes" entries. I will defer to @WeatherGod, however, if he has concrete objections. Same with respect to @tacaswell who might object because this is making a big behavior change.
| # in the superclass, we would go through and actually deal with axis | ||
| # scales and box/datalim. Those are all irrelevant - all we need to do | ||
| # is make sure our coordinate system is square. | ||
| figW, figH = self.get_figure().get_size_inches() |
There was a problem hiding this comment.
I think it makes sense. I have no intention of spending the time required to understand mplot3d well enough to make a detailed review, but in the absence of any such review I am inclined to trust that this PR is a major improvement, as it appears to be fixing a basic design flaw.
I was hoping there was automation for "assume every failed output is an intended change, overwrite the baseline" so that I can diff with git. |
Can you just rsync |
|
No, rsync is not the right tool--you need to explicitly copy the appropriate files from result_images, not all of them. |
|
Certainly you dont want to git add all the comparison files. |
|
triage_tests.py has keyboard shortcuts (run with -h to get the help) to accept images and move between them ("A"/"down"/"A"/"down"/etc should get you there). |
5922edd to
76a36bb
Compare
|
@anntzer: Great tool, thanks. Rebased with updated tests |
9f8cc4c to
a7a8051
Compare
|
I don't understand the circle-ci failures: It seems that travis is failing the svg tests, but the svg files are no longer being generated on my machine. |
|
Thanks for the fixups @anntzer, they look fine to me |
|
What is the status of this? |
There was a problem hiding this comment.
Looking through all of the changed images
- lib/mpl_toolkits/tests/baseline_images/test_mplot3d/bar3d_notshaded.png
- lib/mpl_toolkits/tests/baseline_images/test_mplot3d/scatter3d_color.png
- lib/mpl_toolkits/tests/baseline_images/test_mplot3d/scatter3d.png
- lib/mpl_toolkits/tests/baseline_images/test_mplot3d/surface3d_shaded.png
give me a bit of pause, but I think those are all consequences of the 2.0 default value changes, not due to anything in this PR.
|
I'm not comfortable merging this over @WeatherGod 's requested changes, however I believe they are addressed. |
I've tried to make that easy to look at by having separate commits for the style and other changes - so hopefully you can confirm whether those changes matter. |
| # chosen for similarity with the initial view before gh-8896 | ||
| pb_aspect = np.array([4, 4, 3]) / 3.5 |
There was a problem hiding this comment.
I imagine that there are going to be some requests for an API to change this, for people who liked their 1:1:1 plot boxes. I started working on that two years ago, but it opened a can of worms that I didn't want to open concurrently with this PR. At some point I'll see if I can rebase those changes.
|
Please see https://discourse.matplotlib.org/t/removed-commits-from-master-branch/20868 This merge was "un-done" so that we can re-do it and squash. |
|
Sorry for causing such churn in the repo. I completely forgot to double-check how many image commits there were. |
|
To be clear, is the current status of this "not actually merged"? |
|
Yes. I think you can reopen a PR with the same commits, except that the intermediate images should be stripped out (so yes the intermediate commits won't be testable independently). |
To make bbox_inches='tight' work correctly, we cache the positions of all of the axes, change their aspect to 'auto', adjust the transforms on the figure, render the output, and then restore the previous setting of aspect to each of the figures. This prevent the Axes from trying to adjust their size on draw. This matters because for the render that is emitted the figure transforms are out of sync with the figure size. As part of cleaning fixing the rendering of Axes3D in matplotlib#8896 / matplotlib#16472 we started to use apply_aspect to re-size the area the Axes3D takes up to maintain a fixed ratio between the axes when the aspect is "auto". However this conflicts with the expectation in tight_bbox.adjust_bbox as it assumes setting the aspect to "auto" will prevent any axes resizing. This commit addresses this by: - exiting the Axes3D.apply_aspect early if aspect is auto - adding a new aspect mode 'auto_pb' which is the default for Axes3D which maintains the current master branch behavior of maintaining a fixed ratio between the sizes of the x, y z axis independent of the data limits. - re implement several functions on Axes3D to make sure they handle sharez correctly. closes matplotlib#16463.
The way that bbox_inches='tight' is implemented we need to ensure that we do not try to adjust the aspect during the draw (because we have temporarily de-coupled the reported figure size from the transforms which results in the being distorted). Previously we did not have a way to fix the aspect ratio in screen space of the Axes (only the aspect ratio in dataspace) however in 3.3 we gained this ability for both Axes (matplotlib#14917) and Axes3D (matplotlib#8896 / matplotlib#16472). Rather than add an aspect value to `set_aspect` to handle this case, in the tight_bbox code we monkey-patch the `apply_aspect` method with a no-op function and then restore it when we are done. Previously we would set the aspect to "auto" and restore it in the same places. closes matplotlib#16463.
The way that bbox_inches='tight' is implemented we need to ensure that we do not try to adjust the aspect during the draw (because we have temporarily de-coupled the reported figure size from the transforms which results in the being distorted). Previously we did not have a way to fix the aspect ratio in screen space of the Axes (only the aspect ratio in dataspace) however in 3.3 we gained this ability for both Axes (matplotlib#14917) and Axes3D (matplotlib#8896 / matplotlib#16472). Rather than add an aspect value to `set_aspect` to handle this case, in the tight_bbox code we monkey-patch the `apply_aspect` method with a no-op function and then restore it when we are done. Previously we would set the aspect to "auto" and restore it in the same places. closes matplotlib#16463.
Fixes #8894 and fixes #1104, replacing the bad rendering in the first image, with the good rendering in the second:
Unsurprisingly, this breaks every single unit test,
The plots now all look rather cubic - but that's better than distorted, and we could go back to correctly-scaled rectangles with #8593