Skip to content

Provide axis('equal') for Axes3D (replaces PR #23017) #23409

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

Merged
merged 6 commits into from
Aug 3, 2022

Conversation

scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented Jul 9, 2022

PR Summary

This replaces #23017. As requested in the thread I rebased and added the What's New entry. I think that @patquem will retain the credit for their work in the first commit, please let me know if I did this wrong.

Closes #22570

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@scottshambaugh scottshambaugh changed the title [ENH] : Provide axis('equal') for Axes3D (replace PR #23017) Provide axis('equal') for Axes3D (replaces PR #23017) Jul 9, 2022
@scottshambaugh
Copy link
Contributor Author

This is the What's New image:
image

@tfpf
Copy link
Contributor

tfpf commented Jul 11, 2022

If the user sets limits afterwards, that will also break the equal aspect ratios

Oh, I see now: the aspect ratio will be set for the current view. Makes perfect sense!

There appears to be connection between setting the box aspect to (1, 1, 1) and the new dimensions being (delta, delta, delta). Hacking it might preserve the limits … If I understand set_box_aspect correctly,

v_internals = np.array((self.xaxis.get_view_interval(),
                        self.yaxis.get_view_interval(),
                        self.zaxis.get_view_interval()))
deltas = np.ptp(v_intervals, axis=1)
self.set_box_aspect(deltas)

should do the trick. (Unable to test this right now, so I'll let somebody else decide which approach is better.)

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jul 11, 2022

@tfpf I actually quite like the result with your idea there:

v_intervals = np.vstack((self.xaxis.get_view_interval(),
                         self.yaxis.get_view_interval(),
                         self.zaxis.get_view_interval()))
deltas = np.ptp(v_intervals, axis=1)
self.set_box_aspect(deltas)

# One liner for an existing axes3d `ax`:
# ax.set_box_aspect(np.ptp(np.array([a.get_view_interval() for a in (ax.xaxis, ax.yaxis, ax.zaxis)]), axis=1))

image

I had another idea for how this could look like too, similar to the original idea except it keeps the current (here the default 4:4:3) box aspect ratio:

v_intervals = np.vstack((self.xaxis.get_view_interval(),
                         self.yaxis.get_view_interval(),
                         self.zaxis.get_view_interval()))
mean = np.mean(v_intervals, axis=1)
delta = np.max(np.ptp(v_intervals, axis=1))
deltas = delta * self._box_aspect / min(self._box_aspect)

self.set_xlim3d(mean[0] - deltas[0] / 2., mean[0] + deltas[0] / 2.)
self.set_ylim3d(mean[1] - deltas[1] / 2., mean[1] + deltas[1] / 2.)
self.set_zlim3d(mean[2] - deltas[2] / 2., mean[2] + deltas[2] / 2.)

image

If I had to choose one I think I would choose the last, since the user can precede it with a set_box_aspect((1, 1, 1)) and get the same result as the first. But would welcome input on this. I don't believe matlab has prior art for 3D plots here that we can copy.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jul 11, 2022

So to summarize, here are the three options I see:

  1. Set the box aspect to (1, 1, 1), change axes limits to show equal data aspects
  2. Set the box aspect to be tight around the data, change axes limits to show equal data aspects
  3. Keep the box aspect to the present value, change axes limits to show equal data aspects

I would vote 3 since it encompasses 1 and keeps clear separation between the functions that change the box aspect and the data aspect. Will make the change to 3 unless there are any objections.

Perhaps 2 could be set_box_aspect('tight') or something similar? I'm not too familiar with the 2D analogue for this.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jul 11, 2022

For the PR cleanliness check, I'm not sure how to squash the image commits while making sure @patquem keeps credit for the work in their initial commit.

Edit: I'll just keep them as the author on the squashed commit.

@tfpf
Copy link
Contributor

tfpf commented Jul 11, 2022

I would vote 3 since it encompasses 1, and perhaps 2 could be another keyword? Will make that change unless there are any objections.

This is a nice idea. Could use the adjustable keyword argument—how about:

if adjustable == 'datalim': change the limits, keeping the box aspect the same
if adjustable == 'box': change the box aspect, keeping the limits the same

which would make it more closely resemble the behaviour of 2D axes.

I am not too familiar with this part of the codebase, though. Perhaps somebody else could also provide their suggestions.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jul 11, 2022

I think matching that adjustable keyword is a good idea for API consistency. It looks like a decent bit of effort to build that scaffolding which I don't really have time to do at the moment, so I would say that we keep this as-is and put that in a separate PR. In other words, just doing the adjustable == 'datalim' behavior right now.

Copy link
Contributor

@tfpf tfpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Changing those gallery examples was also a nice touch!

@oscargus
Copy link
Member

Nice discussion, which probably shows why it wasn't implemented earlier: there are lots of definitions of "equal" here...

One thing that struck me, that of course doesn't have to be part of this, is that maybe one would also like to add something like "equalyxy", "equalxz" and "equalyz" to set two out of three axes equal?

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jul 14, 2022

@oscargus good idea, wasn't too hard to add!
Here's the what's new image, setting the aspect after a set_box_aspect((3, 4, 5)):
image

@scottshambaugh scottshambaugh force-pushed the aspect_equal_3d branch 6 times, most recently from face295 to cc9e7ba Compare July 14, 2022 23:25
@oscargus oscargus added this to the v3.6.0 milestone Jul 19, 2022
patquem and others added 3 commits July 21, 2022 19:07
Whats new for 3D plot equal aspect ratio

Code review updates

set_aspect('equal') now adopts current box aspect

Update test image

Update whats new

Update whats new

test image
@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jul 22, 2022

Updated to maximally fill the available space (it was shrunk before), and changed the what's new plotted cuboid to side lengths (1, 1, 5) to exaggerate what's happening:
image

@scottshambaugh
Copy link
Contributor Author

Huh, gallery updates must have gotten lost in a rebase, added back in

@scottshambaugh
Copy link
Contributor Author

As discussed in the dev call yesterday, this is now a blocker for #23449.

@tacaswell tacaswell merged commit 437158c into matplotlib:main Aug 3, 2022
@tacaswell
Copy link
Member

🎉 I'm very excited this is going in!

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.

[ENH]: Provide axis('equal') for Axes3D.
6 participants