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

Consider changing naming convention to indicate squared distances #40

Open
Andlon opened this issue Mar 27, 2023 · 3 comments
Open

Consider changing naming convention to indicate squared distances #40

Andlon opened this issue Mar 27, 2023 · 3 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@Andlon
Copy link

Andlon commented Mar 27, 2023

Just spent some time scratching my head with CollisionConstraints::compute_minimum_distance. The documentation says it returns the minimum distance, but I found this in the implementation:

// NOTE: Actually distance squared

I realize that you're basically working with squared distances throughout (although for an outside user it is not clear if this holds in all cases), but it's still very confusing that "distance", which is a very unambiguously defined term in my opinion, actually means "squared distance". In my opinion, it would reduce confusion a lot for the new users and possible contributors if squared distances were always marked as such, e.g. by using naming like compute_squared_distance, compute_distance2 or similar.

(I labeled this as "bug" as, if you take the documentation and function names at their face value, what is returned is not what the naming suggests)

@Andlon Andlon added the bug Something isn't working label Mar 27, 2023
@zfergus
Copy link
Member

zfergus commented Mar 27, 2023

I see the confusion. Definitely at the very least the documentation for the compute_minimum_distance function needs to be updated to state the returned value is a squared distance.

I originally choose not to put the word squared in the function names to keep them short and concise, and as you said distance in the toolkit is unanimously referring to a squared distance. However, I agree this can be confusing. Let me think about how to best proceed.

@Andlon
Copy link
Author

Andlon commented Mar 28, 2023

Actually, one small counterpoint to the point about squared distances being unanimous: Some parameters are actually not squared, such as dhat and dmin, I believe.

@zfergus
Copy link
Member

zfergus commented Mar 28, 2023

Yes, you are right dhat and dmin are unsquared.

@zfergus zfergus added the enhancement New feature or request label Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants