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

move scipy to test dependencies + remove some usage (mirror #754) #812

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

Conversation

pengzhenghao
Copy link
Member

What changes do you make in this PR?

  • Please describe why you create this PR

resolve #754

Checklist

  • I have merged the latest main branch into current branch.
  • I have run bash scripts/format.sh before merging.
  • Please use "squash and merge" mode.

@pengzhenghao pengzhenghao added the Merge after all tests pass Merge this PR when all tests pass! label Feb 8, 2025
@pengzhenghao
Copy link
Member Author

image

@DhlinV @QuanyiLi
Seems like the point_cloud_lidar depends on the scipy to do fast rotation. But I don't find clear call to the get_rgb_array_cpu from anywhere.

Is it safe to remove scipy dependency?

@DhlinV
Copy link
Collaborator

DhlinV commented Feb 9, 2025

@pengzhenghao I think it's safe since the rotation is used only after the parent class depth computes the depth value. Moving it inside the function shouldn't affect functionality. However, I'm not sure whether it would introduce additional overhead for point cloud-based training.

Finally, I implemented a NumPy version, and it runs without issues on my Linux system. The efficiency doesn't seem to be significantly impacted either, but it might need your double-check. Thanks!

rotation function:

def euler_to_rotation_matrix(hpr):
    """
    Convert ZYX Euler angles to a rotation matrix.
    
    Parameters:
        hpr (array-like): [yaw (Z), pitch (Y), roll (X)] in degrees.
    
    Returns:
        numpy.ndarray: 3x3 rotation matrix.
    """
    hpr = np.radians(hpr)
    
    cz, sz = np.cos(hpr[0]), np.sin(hpr[0])  # Yaw (Z)
    cy, sy = np.cos(hpr[1]), np.sin(hpr[1])  # Pitch (Y)
    cx, sx = np.cos(hpr[2]), np.sin(hpr[2])  # Roll (X)

    rotation_matrix = np.array([
        [cz * cy, cz * sy * sx - sz * cx, cz * sy * cx + sz * sx],
        [sz * cy, sz * sy * sx + cz * cx, sz * sy * cx - cz * sx],
        [-sy,     cy * sx,                cy * cx]
    ])

    return rotation_matrix

replace

from scipy.spatial.transform import Rotation as R
rot = R.from_euler('ZYX', hpr, degrees=True)

rotation_matrix = rot.as_matrix()

by

rotation_matrix = euler_to_rotation_matrix(hpr)

@pengzhenghao
Copy link
Member Author

Cool could you make a commit?

@pengzhenghao
Copy link
Member Author

Actually I don't know who've ever called this function. Do we have unit test / profile for this function?

@DhlinV
Copy link
Collaborator

DhlinV commented Feb 9, 2025

@pengzhenghao Sorry for the delay, I just committed a pull request. And I think this function is called when running "metadrive/examples/point_cloud_lidar.py" (like a test script)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge after all tests pass Merge this PR when all tests pass!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants