Skip to content

feat: Add ProbabilisticKeyPoint3D #171

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kunlin596
Copy link

No description provided.

@kunlin596 kunlin596 force-pushed the kun/feat/add-probabilistic-key-point-3d branch 3 times, most recently from 52dc7a8 to b7958ec Compare April 9, 2025 09:40
assert np.allclose(mat, mat.T, rtol=rtol, atol=atol), f"{mat} is not symmetric!"

@staticmethod
def _assert_positive_semi_definite(mat: np.ndarray) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is actually checking for positive definite. If we want to allow the zero matrix (the default value) we have to allow 0 eignvalues.

Also, is this enough to conclude the diagonal elements are always non negative?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are right, this is actually positive definite. If for instance the var_z is zero it means that in the Z direction there is no variance which is not realistic, I will revise the function name.

Also yes, the diagonal elements are guaranteed to be non-negative.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, but the default value in the proto will be 0, so I think we should either support the zero matrix (which has 0 eigenvalues) or we need to set a default value in the proto (via default keyword) that is large.

Copy link
Author

Choose a reason for hiding this comment

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

I set the matrix validation check to False by default. Also it looks like we could not set default value in proto3. In this case I think it would make more sense to ensure the value on the caller side.

@kunlin596 kunlin596 force-pushed the kun/feat/add-probabilistic-key-point-3d branch 2 times, most recently from f520398 to d9920df Compare April 10, 2025 08:57
@kunlin596 kunlin596 marked this pull request as ready for review April 10, 2025 08:58
@kunlin596 kunlin596 force-pushed the kun/feat/add-probabilistic-key-point-3d branch 6 times, most recently from efd5679 to 21b9d0a Compare April 14, 2025 04:50
@kunlin596 kunlin596 requested a review from chrisochoatri April 14, 2025 04:51
@kunlin596 kunlin596 force-pushed the kun/feat/add-probabilistic-key-point-3d branch 3 times, most recently from b0e7445 to 7c30ad0 Compare April 15, 2025 04:08
@kunlin596 kunlin596 force-pushed the kun/feat/add-probabilistic-key-point-3d branch from 7c30ad0 to c8dd7fe Compare April 15, 2025 04:14
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