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

mapping.icp doesn't return the minimizing permutation of indices #15

Closed
bdice opened this issue Jun 4, 2019 · 2 comments · Fixed by #19
Closed

mapping.icp doesn't return the minimizing permutation of indices #15

bdice opened this issue Jun 4, 2019 · 2 comments · Fixed by #19
Assignees
Labels
enhancement New feature or request

Comments

@bdice
Copy link
Member

bdice commented Jun 4, 2019

When using rowan.mapping.icp, the algorithm returns a matrix R and a translation t that map the points in set X to set Y. However, if the points in X and Y are permuted (re-ordered), then it's not enough to rotate and translate the points - they must also be permuted.

The permutation of indices that minimizes the mapping is used internally in the Iterative Closest Point algorithm, and it would be helpful to return those indices alongside the rotation matrix R and translation t.

@bdice bdice self-assigned this Jun 4, 2019
@bdice bdice added the enhancement New feature or request label Jun 4, 2019
@vyasr
Copy link
Collaborator

vyasr commented Jun 13, 2019

As we discussed offline, the internal indices are just based on the closest points found when comparing the two configurations. The main value in returning the indices is simply to provide a way to more effectively measure how good the match was. I'm open to returning the indices based on an optional argument (i.e. adding return_indexes=False to the signature) if there's a desire to have that available.

@vyasr
Copy link
Collaborator

vyasr commented Jul 8, 2019

@bdice what are your thoughts? I'm not really convinced that this is necessary, but since it's essentially free I would be fine with a PR to optionally return the indexes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants