Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix mplot3d projection#8896
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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]) | |||
eric-wieserJul 16, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 to5922eddComparePlot 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why use the figure's aspect ratio? What if the figure has 3 subplots all in a row?
eric-wieserAug 2, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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> . |
Uh oh!
There was an error while loading.Please reload this page.
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) |
eric-wieserDec 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Bug fixed here on fractional view angles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Extracted to#12573. I misremembered - the crash was only for non-finite values.
darkdragon-001 commentedMar 23, 2019
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 to76a36bbCompare@anntzer: Great tool, thanks. Rebased with updated tests |
9f8cc4c toa7a8051CompareI 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? |
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 seehttps://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 ofall of the axes, change their aspect to 'auto', adjust the transformson the figure, render the output, and then restore the previoussetting of aspect to each of the figures. This prevent the Axes fromtrying to adjust their size on draw. This matters because for therender that is emitted the figure transforms are out of sync with thefigure size.As part of cleaning fixing the rendering of Axes3D inmatplotlib#8896 /matplotlib#16472we started to use apply_aspect to re-size the area the Axes3D takes upto maintain a fixed ratio between the axes when the aspect is "auto".However this conflicts with the expectation in tight_bbox.adjust_bboxas it assumes setting the aspect to "auto" will prevent any axesresizing.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.closesmatplotlib#16463.
The way that bbox_inches='tight' is implemented we need to ensure thatwe do not try to adjust the aspect during the draw (because we havetemporarily de-coupled the reported figure size from the transformswhich results in the being distorted). Previously we did not have away to fix the aspect ratio in screen space of the Axes (only theaspect ratio in dataspace) however in 3.3 we gained this ability forboth 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 witha no-op function and then restore it when we are done. Previously wewould set the aspect to "auto" and restore it in the same places.closesmatplotlib#16463.
The way that bbox_inches='tight' is implemented we need to ensure thatwe do not try to adjust the aspect during the draw (because we havetemporarily de-coupled the reported figure size from the transformswhich results in the being distorted). Previously we did not have away to fix the aspect ratio in screen space of the Axes (only theaspect ratio in dataspace) however in 3.3 we gained this ability forboth 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 witha no-op function and then restore it when we are done. Previously wewould set the aspect to "auto" and restore it in the same places.closesmatplotlib#16463.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#8894 andfixes#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