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]: class Axes3D.set_box_aspect() sets wrong aspect ratios when Axes3D.view_init( vertical_axis='y') is enabled. #24328

Closed
Michallote opened this issue Nov 1, 2022 · 7 comments · Fixed by #28041
Milestone

Comments

@Michallote
Copy link

Bug summary

Whenever creating a 3D plot with vertical_axis = 'y' option the set_box_aspect() method scales the axes incorrectly. The 3-tuple is expected to behave as x:y:z, instead it behaves like z:x:y

Code for reproduction

"""
Created on Mon Oct 31 18:30:31 2022

@author: Michel Gordillo
"""

import numpy as np
import matplotlib.pyplot as plt
from matplotlib import cm

fig = plt.figure(num=4, clear=True)
ax = fig.add_subplot(1, 1, 1, projection='3d')
ax.view_init(vertical_axis='y')

(theta, phi) = np.meshgrid(np.linspace(0, 2 * np.pi, 41),
                           np.linspace(0, 2 * np.pi, 41))

x = (3 + np.cos(phi)) * np.cos(theta)
z = (3 + np.cos(phi)) * np.sin(theta)
y = np.sin(phi)

k = 4

xlim = [-k, k]  #2k
ylim = [-k/2, k/2] #k
zlim = [-k, k] #2k

box_aspect = (4,2,4) #(x,y,z)

def fun(t, f): return (np.cos(f + 2 * t) + 1) / 2


dplot = ax.plot_surface(x, y, z, facecolors=cm.jet(fun(theta, phi)))
ax.set(xlabel='x', 
       ylabel='y', 
       zlabel='z',
       xlim = xlim,
       ylim = ylim,
       zlim = zlim,
       # xticks = [-4, -2, 2, 4],
       # yticks = [-4, -2, 2, 4],
       # zticks = [-1, 0, 1],
       title='Donut!')

ax.set_box_aspect(aspect = box_aspect)

fig.tight_layout()

Actual outcome

Matplotlib_bug_axes_vertical_y

The box_aspect tuple:
box_aspect = (4,2,4) #(x,y,z)

Actually the axis scaled as
box_aspect = (4,2,4) -> #(z,x,y)
Like if shifting y to the right.

Expected outcome

Matplotlib_bug_fix_axes_vertical_y

To produce this result i just applied the same logic in reverse:
box_aspect = (4,4,2) #(z,x,y) <- Expecting the outcome to be applied like this

And it actually does.

Additional information

This only happens as explained in the title when both vertical = y option is enabled and we try to manually scale_box_aspect(). The problem is probably from how was vertical axis selection actually implemented.

From the documentation:

set_box_aspect(aspect, *, zoom=1)
[...]
aspect3-tuple of floats or None
Changes the physical dimensions of the Axes3D, such that the ratio of the axis lengths in display units is x:y:z.

This behaviour is not respected when vertical_axis = 'y' in enabled. The behaviour appears to be: z:x:y instead.

To actually fix this I just created a tuple with the new definition in mind, this could just be patched by shifting the tuple to make the vertical axis be the last element of the tuple

(x,y,z) -> input tuple

vertical axis == 'z' -> (x,y,z) no problem
vertical axis == 'y' -> (z,x,y) rotate right the tuple
vertical axis == 'x' -> (y,z,x) rotate right twice the tuple

Operating system

Windows

Matplotlib Version

'3.6.1'

Matplotlib Backend

Qt5Agg

Python version

Python 3.8.5

Jupyter version

Spyder 4.1.5

Installation

conda

@oscargus
Copy link
Contributor

oscargus commented Nov 1, 2022

Thanks! I do not recall that vertical_axis was discussed during the introduction of 3D aspect ratios, so not surprised that it happens. Feel free to open a PR if you have a solution, it seems like what you have proposed is most likely the correct fix if applied in the correct location.

@Michallote
Copy link
Author

Yeah I'd love to! This would be my first time commit a PR so I still have to read the guidelines and figure out what to do here on github. Thank you

@oscargus
Copy link
Contributor

oscargus commented Nov 3, 2022

Sounds great!

I was wrong about the history of the methods, but it seems like the vertical_axis is stored as self._vertical_axis with an integer number according to x=0, y=1, z=2. So probably you would like to rotate aspect just before here:

self._box_aspect = aspect

@Michallote
Copy link
Author

Thank you, I was browsing through the files today and found that line of code but couldn't figure out the structure of the vertical axis. My solution so far is not very elegant but does the trick: Before setting ax.view_init I save the variable elsewhere.

vertical_axis = 'y'
ax.view_init( vertical_axis = vertical_axis )
        xlim = [min_x, max_x]
        ylim = [min_y, max_y]
        zlim = [min_z, max_z]
        
        def interval(x): return x[1] - x[0] #Get the total 'length' of an axis
        
        lims = [xlim , ylim , zlim]
        lims_len = [interval(lim) for lim in lims] #Get the length of each axis
        
        k = np.min(lims_len)

        box_aspect = tuple(lims_len/k)
        
        axisroll = {'x':2,'y':1,'z':0}

        if axisroll[vertical_axis ]:
            box_aspect = tuple(np.roll(box_aspect,shift = axisroll[vertical_axis ]))

@Michallote
Copy link
Author

Michallote commented Nov 5, 2022

I think I found the problem:

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) # <-- The issue is here! box_aspect doesn't gets reassigned to self.
        worldM = proj3d.world_transformation(
            *self.get_xlim3d(),
            *self.get_ylim3d(),
            *self.get_zlim3d(),
            pb_aspect=box_aspect,
        )

In order to solve the problem just adding after calling roll to vertical would work.
self.box_aspect = box_aspect

@scottshambaugh
Copy link
Contributor

Hi @Michallote, are you still interested in making a PR for this? We have a guide on opening a PR for new contributors at https://matplotlib.org/stable/devel/contributing.html#contributing-code, and feel free to hop on the gitter chat if you have more questions!

@Illviljan
Copy link
Contributor

I made an attempt in #28041. The idea is if the x-axis has the smallest aspect it should always be the smallest regardless of what vertical_axis says.
So when setting box_aspect we have to go the opposite direction as _roll_to_vertical would do, pretty much what oscargus suggested.

@QuLogic QuLogic added this to the v3.9.1 milestone Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants