-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Changes from 7 commits
717e20e
df6dcb1
349975d
2db9d1a
2f71739
11d996f
373651f
3589da5
491395a
0080cd9
8833023
9f6354a
f904414
2f49d87
f7259e4
68b26df
57c7a65
1137dce
7405597
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -383,7 +383,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): | ||
|
@@ -1190,15 +1190,16 @@ 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): | ||
"""Roll arrays to match the different vertical axis.""" | ||
return np.roll(arr, self._vertical_axis - 2) | ||
return np.roll(arr, sign * (self._vertical_axis - 2)) | ||
|
||
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 commentThe 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 commentThe 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).
Illviljan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
worldM = proj3d.world_transformation( | ||
*self.get_xlim3d(), | ||
*self.get_ylim3d(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2250,6 +2250,23 @@ def test_view_init_vertical_axis( | |
np.testing.assert_array_equal(tickdir_expected, tickdir_actual) | ||
|
||
|
||
@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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I think there are two ways to make this better: Either
|
||
|
||
|
||
@image_comparison(baseline_images=['arc_pathpatch.png'], | ||
remove_text=True, | ||
style='mpl20') | ||
|
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.
The naming
sign
is purely technical but does not explain what it's doing.