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

[BUG]: Shift box_aspect according to vertical_axis #28041

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented Apr 7, 2024

PR summary

Make sure box_aspect stays the same for each axis when using set_box_aspect and vertical_axis is used.

import matplotlib.pyplot as plt
import numpy as np


x = np.array([0, 1, 2, 4])
y = np.array([5, 10])
z = np.array([100, 150, 200])
X, Y, Z = np.meshgrid(*[x, y, z], indexing="ij")
c = np.arange(0, X.size)

plt.figure()
for j, (a, e) in enumerate([(0, 0), (-60, 30)]):
    for i, vert_a in enumerate(["z", "y", "x"]):
        ax = plt.subplot(2, 3, j * 3 + 1 + i, projection="3d")
        pc = ax.scatter(X, Y, Z, c=c)
        ax.view_init(azim=a, elev=e, vertical_axis=vert_a)
        ax.set_title(f"azim={a}, elev={e}, vertical_axis='{vert_a}'")
        ax.set_xlabel("x")
        ax.set_ylabel("y")
        ax.set_zlabel("z")
        ax.set_box_aspect((1, 2, 3))

Before PR:
image

With PR:
image

PR checklist

@Illviljan Illviljan marked this pull request as ready for review April 14, 2024 13:42
@Illviljan
Copy link
Contributor Author

Illviljan commented Apr 14, 2024

Fixes set_aspect as well, since it uses set_box_aspect.

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]))

With PR:
image

Copy link
Contributor

@scottshambaugh scottshambaugh left a 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 Show resolved Hide resolved

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
Copy link
Contributor

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

Copy link
Contributor Author

@Illviljan Illviljan Apr 17, 2024

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>
Copy link
Contributor

@scottshambaugh scottshambaugh left a 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!

@scottshambaugh
Copy link
Contributor

It looks like this needs a rebase to deconflict with your other MR that was just merged in.

@scottshambaugh scottshambaugh added this to the v3.9.1 milestone May 14, 2024
@scottshambaugh scottshambaugh changed the title Shift box_aspect according to vertical_axis [BUG]: Shift box_aspect according to vertical_axis May 14, 2024
@@ -1191,9 +1191,9 @@ def set_proj_type(self, proj_type, focal_length=None):
f"None for proj_type = {proj_type}")
self._focal_length = np.inf

def _roll_to_vertical(self, arr):
def _roll_to_vertical(self, arr, sign=1):
Copy link
Member

Choose a reason for hiding this comment

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

The naming sign is purely technical but does not explain what it's doing.

Suggested change
def _roll_to_vertical(self, arr, sign=1):
def _roll_to_vertical(self, arr, reverse=False):
"""
Roll arrays to match the different vertical axis.
If *reverse*, perform the inverse operation.
"""

Comment on lines 2280 to 2293
def test_set_box_aspect_vertical_axis(vertical_axis: str) -> None:
ax = plt.subplot(1, 1, 1, projection="3d")
ax.view_init(elev=0, azim=0, roll=0, vertical_axis=vertical_axis)
ax.figure.canvas.draw()

aspect_old = tuple(ax._box_aspect)
aspect_expected = np.roll(
aspect_old, -1 * (ax._axis_names.index(vertical_axis) - 2)
)

ax.set_box_aspect(None)
aspect_new = tuple(ax._box_aspect)

np.testing.assert_allclose(aspect_expected, aspect_new)
Copy link
Member

Choose a reason for hiding this comment

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

Does this test have semantic meaning? I can't see by itself that aspect_expected is expected to be as defined by the roll operation. To me it appears that this formular was just copied from set_box_aspect, which would be tautological ("set_box_aspect behaves according to the formula given in set_box_aspect") and doesn't actually prove anything. Also note that the first two entries of the default box aspect (None -> (4, 4, 3)) are identical. This reduces test power because we cannot distinguish whether they are mixed up or not.

I think there are two ways to make this better: Either

  • Use explicit expectations. Something like (not checked the numbers and the exact logic):
    ax.view_init(elev=0, azim=0, roll=0, vertical_axis="x")
    ax.set_box_aspect((1, 2, 3))  # (x, y, z)
    np.testing.assert_allclose)(ax._box_aspect, (2, 3, 1))  # (width, depth, height)
    
    or
  • use an image comparison test and compare rotated views with permuted box_aspects.

Comment on lines 1205 to 1207
sign : Literal[1, -1], default: 1
Roll the array elements in a positive or negative
direction. Defaults to the positive direction.
Copy link
Member

Choose a reason for hiding this comment

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

My point was that the naming sign itself is not good because it's on a technical level not reflecting the underlying semantics. Expanding the documentation of the technical action only marginally improves understandability. To be explicit: Please change the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sign was inspired by np.sign for reference.
The arguments I see against reverse: bool is that it leads to more code complexity (an additional if-check). This function is triggered every time on move so some performance consideration has to be made in my opinion. Probably not the worst bottleneck though.

Comment on lines 1195 to 1197
def _roll_to_vertical(
self, arr: "np.typing.ArrayLike", sign: Literal[1, -1] = 1
) -> np.ndarray:
Copy link
Member

Choose a reason for hiding this comment

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

@QuLogic it seems mplot3d is not yet typed at all. Do you plan to go with type stubs or inline type hints here?

Copy link
Member

Choose a reason for hiding this comment

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

Correct that all of mpltoolkits (including mplot3d) is not yet type hinted at all.

I have no current plans regarding extending type hints here at the moment (not opposed, just have no active plans to undertake it myself)

I think whether to do stubs or inline is up to whoever wishes to implement it. Stubs would be my default option, just because that is what we have mostly, but it is not a strong opinion.

I do prefer inline in general, but opted for stubs because it limited how much runtime code needed to be touched in an already pretty large task.

That said, we have precedence for some new typing work being done inline (_mathtext.py).

Further, I have in general erred towards not type hinting private functionality (as this is), at least in stubs, as they are mostly useful to external users anyway.

TL;DR Could go either way, not really harming anything to have them here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants