8000 [BUG]: Shift box_aspect according to vertical_axis by Illviljan · Pull Request #28041 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

[BUG]: Shift box_aspect according to vertical_axis #28041

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.

Sign up for GitHub

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 19 commits into from
Jun 2, 2024
Merged

[BUG]: Shift box_aspect according to vertical_axis #28041

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
717e20e
Update axes3d.py
Illviljan Apr 7, 2024
df6dcb1
missed paranthesis
Illviljan Apr 7, 2024
349975d
Add test
Illviljan Apr 14, 2024
2db9d1a
Merge branch 'main' into vertical_axis_box_aspect
Illviljan Apr 14, 2024
2f71739
Update test_axes3d.py
Illviljan Apr 14, 2024
11d996f
Update test_axes3d.py
Illviljan Apr 14, 2024
373651f
Update lib/mpl_toolkits/mplot3d/axes3d.py
Illviljan Apr 17, 2024
3589da5
Update lib/mpl_toolkits/mplot3d/axes3d.py
Illviljan Apr 17, 2024
491395a
Merge branch 'main' into vertical_axis_box_aspect
Illviljan May 5, 2024
0080cd9
Merge branch 'main' into vertical_axis_box_aspect
Illviljan May 5, 2024
8833023
Merge branch 'main' into vertical_axis_box_aspect
Illviljan May 11, 2024
9f6354a
Merge branch 'main' into vertical_axis_box_aspect
Illviljan May 15, 2024
f904414
Merge branch 'main' into vertical_axis_box_aspect
Illviljan May 20, 2024
2f49d87
Expand docstring
Illviljan May 22, 2024
f7259e4
Explicit test
Illviljan May 28, 2024
68b26df
rename sign to reverse
Illviljan May 28, 2024
57c7a65
Update axes3d.py
Illviljan May 28, 2024
1137dce
correct expected values according to ci
Illviljan May 29, 2024
7405597
Update lib/mpl_toolkits/mplot3d/tests/test_axes3d.py
timhoffm Jun 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions lib/mpl_toolkits/mplot3d/axes3d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"""

from collections import defaultdict
from typing import Literal
import itertools
import math
import textwrap
Expand Down Expand Up @@ -383,7 +384,7 @@ def set_box_aspect(self, aspect, *, zoom=1):
# of the axes in mpl3.8.
aspect *= 1.8294640721620434 * 25/24 * zoom / np.linalg.norm(aspect)

self._box_aspect = aspect
self._box_aspect = self._roll_to_vertical(aspect, sign=-1)
self.stale = True

def apply_aspect(self, position=None):
Expand Down Expand Up @@ -1191,9 +1192,21 @@ 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):
"""Roll arrays to match the different vertical axis."""
return np.roll(arr, self._vertical_axis - 2)
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
8000 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.

"""
Roll arrays to match the different vertical axis.

Parameters
----------
arr : ArrayLike
Array to roll.
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 d 8000 escribe 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.

"""
return np.roll(arr, sign * (self._vertical_axis - 2))

def get_proj(self):
"""Create the projection matrix from the current viewing position."""
Expand Down
17 changes: 17 additions & 0 deletions lib/mpl_toolkits/mplot3d/tests/test_axes3d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2276,6 +2276,23 @@ def test_on_move_vertical_axis(vertical_axis: str) -> None:
)


@pytest.mark.parametrize("vertical_axis", ["x", "y", "z"])
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.



@image_comparison(baseline_images=['arc_pathpatch.png'],
remove_text=True,
style='mpl20')
Expand Down
0