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

[Enhance] Faster implementation of KNN #586

Merged
merged 4 commits into from
May 26, 2021

Conversation

Wuziyi616
Copy link
Contributor

@Wuziyi616 Wuziyi616 commented May 25, 2021

This new KNN code is borrowed from PAConv. They use heap data structure to accelerate so it's called knn_heap.

points.shape == [8, 4096, 3]
centers.shape == [8, 64, 3]
k = 32
100 loops:
The current knn needs ~8s, while knn_heap only needs < 0.1s!

Should we still keep the current knn in the codebase? After all, they have the same functionality. And knn is not used in our codebase now so this won't cause any BC-breaking. (Or we can do a import knn_heap as knn trick in mmdet3d/ops/__init__.py?)

@Wuziyi616
Copy link
Contributor Author

My first time working with custom gpu ops. Please point out any mistakes I made.

@codecov
Copy link

codecov bot commented May 25, 2021

Codecov Report

Merging #586 (6abea67) into master (d37c330) will increase coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #586      +/-   ##
==========================================
+ Coverage   50.77%   50.81%   +0.04%     
==========================================
  Files         197      197              
  Lines       14923    15052     +129     
  Branches     2426     2451      +25     
==========================================
+ Hits         7577     7649      +72     
- Misses       6851     6893      +42     
- Partials      495      510      +15     
Flag Coverage Δ
unittests 50.81% <0.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmdet3d/ops/knn/knn.py 30.00% <0.00%> (-2.15%) ⬇️
mmdet3d/apis/inference.py 53.62% <0.00%> (-5.58%) ⬇️
mmdet3d/core/bbox/structures/coord_3d_mode.py 63.12% <0.00%> (-1.85%) ⬇️
mmdet3d/core/visualizer/show_result.py 66.38% <0.00%> (-0.57%) ⬇️
mmdet3d/apis/test.py 15.78% <0.00%> (-0.43%) ⬇️
mmdet3d/datasets/nuscenes_dataset.py 40.70% <0.00%> (-0.29%) ⬇️
mmdet3d/apis/__init__.py 100.00% <0.00%> (ø)
mmdet3d/core/bbox/structures/utils.py 97.05% <0.00%> (+0.69%) ⬆️
mmdet3d/models/detectors/single_stage_mono3d.py 16.48% <0.00%> (+1.20%) ⬆️
mmdet3d/datasets/nuscenes_mono_dataset.py 38.27% <0.00%> (+4.94%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d37c330...6abea67. Read the comment docs.

@ZwwWayne
Copy link
Collaborator

If the input and output of these two modules are the same, we can simply replace the original one for acceleration. Otherwise, we might need to prevent BC-breaking.

@Wuziyi616
Copy link
Contributor Author

If the input and output of these two modules are the same, we can simply replace the original one for acceleration. Otherwise, we might need to prevent BC-breaking.

Yes, I modify the wrapper so that the input and output are the same now. Also, knn is not used in our current codebase (I understand some users may use it in their projects). I will replace the original one.

@yezhen17
Copy link
Collaborator

Maybe add a line somewhere telling users the speed difference of two implementations?

@Wuziyi616
Copy link
Contributor Author

Maybe add a line somewhere telling users the speed difference of two implementations?

I will just replace the old one with the new KNN op since their interface are the same.

@ZwwWayne ZwwWayne merged commit 53271e3 into open-mmlab:master May 26, 2021
@Wuziyi616 Wuziyi616 deleted the paconv_knn_heap branch May 27, 2021 05:41
tpoisonooo pushed a commit to tpoisonooo/mmdetection3d that referenced this pull request Sep 5, 2022
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.

3 participants