Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Tensor FPFH #5429

Merged
merged 16 commits into from
Aug 18, 2022
Merged

Add Tensor FPFH #5429

merged 16 commits into from
Aug 18, 2022

Conversation

yuecideng
Copy link
Collaborator

@yuecideng yuecideng commented Aug 12, 2022

This PR adds tensor ComputeFPFHFeature and use bunny mesh for unit test.

This change is Reviewable

@update-docs
Copy link

update-docs bot commented Aug 12, 2022

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@yuecideng yuecideng marked this pull request as ready for review August 12, 2022 14:44
Copy link
Contributor

@theNded theNded left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 15 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @reyanshsolis, @ssheorey, @yuecideng, and @yxlao)


.clang-format line 5 at r2 (raw file):

ColumnLimit: 80
UseTab: Never
# Standard: c++14

Is this change necessary? It affects formatting.

@yuecideng
Copy link
Collaborator Author

It was changed probably when I tried to fix the formatting issue in my PC. I will remove it anyway... @theNded

@yuecideng yuecideng requested a review from theNded August 15, 2022 10:45
Copy link
Contributor

@theNded theNded left a comment

Choose a reason for hiding this comment

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

:lgtm:

Would you please also attach the benchmark log in this PR for reference?

@reyanshsolis would you please create a following PR on matching tensor FPFH using the tensor nns interface, if you have any bandwidth?
I also notice nns does not have pybind as of now if I understand correctly. Would this be an obstacle to implementation?

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @reyanshsolis, @ssheorey, @yuecideng, and @yxlao)

@yuecideng
Copy link
Collaborator Author

yuecideng commented Aug 16, 2022

Update

image

The parameters of nns search is sensitive to the time cost of comuputing fpfh with tensor API. We should avoid using large max_nn on tensor cuda. @theNded @reyanshsolis

Copy link
Collaborator

@yxlao yxlao left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 15 files at r1, 1 of 6 files at r2, 1 of 4 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @reyanshsolis, @ssheorey, and @yuecideng)

@yxlao yxlao merged commit 8a506d9 into master Aug 18, 2022
@yxlao yxlao deleted the yueci/tensor-fpfh branch August 18, 2022 00:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants