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

Fix convexhull boundary computation #244

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

johnwlambert
Copy link
Contributor

@johnwlambert johnwlambert commented Jun 24, 2021

Switch to fully using scipy (8.3k stars) instead of quaternion (413 stars) for rotation matrix -> quaternion conversions.

Note: the quaternion library uses scalar-first order qw, qx, qy, qz (see this issue), whereas scipy uses scalar-last order, so we re-order the coefficients inside yaw_to_quaternion3d().

Resolves the issue mentioned in #239.

argoverse/evaluation/competition_util.py Outdated Show resolved Hide resolved
@@ -48,6 +55,13 @@ def quat_argo2scipy(q: np.ndarray) -> np.ndarray:
return q_scipy


def quat_scipy2argo(q: np.ndarray) -> np.ndarray:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is on a bit of a meta note, but we really need to clean up transform.py (the file as a whole).

@johnwlambert johnwlambert changed the title Remove quaternion package dependency Remove `quaternion' package dependency Jun 26, 2021
@johnwlambert johnwlambert changed the title Remove `quaternion' package dependency Remove 'quaternion' package dependency Jun 26, 2021
@johnwlambert
Copy link
Contributor Author

@tagarwal-argoai would you mind taking a look at this, also?

return Polygon(poly)
# `simplices` contains indices of points forming the simplical facets of the convex hull.
poly_pts = hull.points[np.unique(hull.simplices)]
return Polygon(poly_pts)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janesjanes and @benjaminrwilson would you mind taking a brief look at this?

I believe the previous code was actually mixing the x,y,z coordinates of 3 vertices, which I don't think was the intended behavior.

If a simplicial facet is defined by vertex 0 = (x0,y0,z0), vertex 1=(x1,y1,z1), vertex 2=(x2,y2,z2), then I think we were forming (x0,x1,x2) as a point of the polygon, which wouldn't be correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for looking into this, @johnwlambert. Before I dive in, are we using this function anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Ben. So this is used in a tracking competition demo notebook: https://github.com/argoai/argoverse-api/blob/master/demo_usage/competition_tracking_tutorial.ipynb

return (pt1[0] - pt0[0]) / dis_0_to_1, (pt1[1] - pt0[1]) / dis_0_to_1

def poly_to_label(poly: Polygon, category: str = "VEHICLE", track_id: str = "") -> ObjectLabelRecord:
"""Convert a Shapely Polygon to a 3d cuboid by estimating the minimum-bounding rectangle.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to simplify this logic a bit here, and make things a bit clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this get called outside of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"x": label.quaternion[0],
"y": label.quaternion[1],
"z": label.quaternion[2],
"w": label.quaternion[3],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janesjanes since we express quaternions as qw, qx, qy, qz, I'm not sure this is correct here.

Since the quaternion package uses qw, qx, qy, qz, but we are unpacking this as qx, qy, qz, qw.

# z = poly.exterior.xy[2]

d1 = dist((x[0], y[0]), (x[1], y[1]))
d2 = dist((x[1], y[1]), (x[2], y[2]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought this indexing was a bit hard to follow, so i tried to simplify it a bit

@@ -220,7 +206,7 @@ def poly_to_label(poly: Polygon, category: str = "VEHICLE", track_id: str = "")


def get_objects(clustering: DBSCAN, pts: np.ndarray, category: str = "VEHICLE") -> List[Tuple[np.ndarray, uuid.UUID]]:

""" """
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we provide a small docstring here?

@johnwlambert johnwlambert changed the title Remove 'quaternion' package dependency Fix convexhull boundary computation and remove 'quaternion' package dependency Jun 30, 2021
@johnwlambert johnwlambert changed the title Fix convexhull boundary computation and remove 'quaternion' package dependency Fix convexhull boundary computation Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants