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
[BUG]: Shift box_aspect according to vertical_axis #28041
base: main
Are you sure you want to change the base?
Conversation
Fixes import numpy as np
import matplotlib.pyplot as plt
import mpl_toolkits.mplot3d as a3
plt.close('all')
fig = plt.figure()
ax = fig.add_subplot(111, projection='3d')
ax.view_init(vertical_axis='y')
ax.set_xlabel('X')
ax.set_ylabel('Y')
ax.set_xlabel('Z')
ax.axes.set_xlim3d(left=-1, right=3.5)
ax.axes.set_ylim3d(bottom=0, top=10)
ax.axes.set_zlim3d(bottom=-0.5, top=3.5)
ax.set_aspect('equal')
def a(elemList):
return np.array(elemList)
def drawTriangle(centre, size=1, color='y'):
# A neuron is a triangle
left = centre + a([- size / 2, - size / 2, 0])
right = centre + a([size / 2, - size / 2, 0])
top = centre + a([0, size / 2, 0])
coords = a([left, top, right])
tri = a3.art3d.Poly3DCollection([coords])
tri.set_color(color)
tri.set_edgecolor('k')
ax.add_collection3d(tri)
for col in range(3):
drawTriangle(a([col, 10, 0])) |
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.
Thanks for fixing this as well! Two small notes, then I can approve the MR.
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
|
||
def get_proj(self): | ||
"""Create the projection matrix from the current viewing position.""" | ||
|
||
# Transform to uniform world coordinates 0-1, 0-1, 0-1 | ||
box_aspect = self._roll_to_vertical(self._box_aspect) | ||
# self._box_aspect = box_aspect |
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.
What's the reason for removing this line? I'm not totally clear where it comes into play (when redrawing somewhere?), but note that the 3D test coverage isn't amazing so we should understand where behavior is changing. I would make sure that the plots are behaving as expected when you manually rotate the view.
Should remove rather than comment out if it's unneeded
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.
It never was in main. I was just testing different ideas here.
Conclusion here is that box_aspect has to be local, otherwise there will be a new vertical axis each click (or call to get_proj).
Co-authored-by: Scott Shambaugh <14363975+scottshambaugh@users.noreply.github.com>
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.
CI failure looks unrelated to me, good to merge with a second reviewer!
It looks like this needs a rebase to deconflict with your other MR that was just merged in. |
PR summary
Make sure box_aspect stays the same for each axis when using
set_box_aspect
and vertical_axis is used.Before PR:
With PR:
PR checklist