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] Remove mmcv.iou3d from mmdetecion3d #1403

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

filaPro
Copy link
Contributor

@filaPro filaPro commented Apr 16, 2022

Motivation

Remove mmcv.iou3d from mmdetection3d.

  • iou3d_boxes_overlap_bev_forward -> box_iou_rotated.box_iou_rotated
  • iou_3d.nms_bev -> nms.nms_rotated
  • iou_3d.nms_normal_bev -> nms.nms

Modification

For now only iou3d_boxes_overlap_bev_forward -> box_iou_rotated.box_iou_rotated is done. Nms can be replaced in this PR in the future or in the other PR.

BC-breaking (Optional)

May be only due to some float tolerance.

@codecov
Copy link

codecov bot commented Apr 16, 2022

Codecov Report

Merging #1403 (3f38b02) into dev (4b73f74) will decrease coverage by 0.03%.
The diff coverage is 29.54%.

@@            Coverage Diff             @@
##              dev    #1403      +/-   ##
==========================================
- Coverage   51.20%   51.17%   -0.04%     
==========================================
  Files         212      212              
  Lines       18280    18284       +4     
  Branches     2978     2980       +2     
==========================================
- Hits         9361     9356       -5     
- Misses       8362     8371       +9     
  Partials      557      557              
Flag Coverage Δ
unittests 51.17% <29.54%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
mmdet3d/core/post_processing/box3d_nms.py 28.76% <22.22%> (-1.54%) ⬇️
mmdet3d/models/dense_heads/point_rpn_head.py 27.02% <25.00%> (-0.31%) ⬇️
...odels/roi_heads/bbox_heads/point_rcnn_bbox_head.py 30.49% <25.00%> (-0.18%) ⬇️
mmdet3d/core/bbox/structures/base_box3d.py 71.57% <33.33%> (-0.15%) ⬇️
mmdet3d/core/post_processing/merge_augs.py 8.00% <33.33%> (-1.81%) ⬇️
mmdet3d/models/dense_heads/parta2_rpn_head.py 15.15% <33.33%> (-1.52%) ⬇️
...3d/models/roi_heads/bbox_heads/parta2_bbox_head.py 68.35% <33.33%> (-0.14%) ⬇️
mmdet3d/models/dense_heads/centerpoint_head.py 44.73% <50.00%> (ø)
mmdet3d/core/post_processing/__init__.py 100.00% <100.00%> (ø)

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 4b73f74...3f38b02. Read the comment docs.

@filaPro
Copy link
Contributor Author

filaPro commented Apr 17, 2022

I've also checked that

  • iou_3d.nms_bev is same with nms.nms_rotated;
  • iou_3d.nms_normal_bev is same with nms.nms.

So, I plan to add this replacements to the current PR.

Copy link
Member

@Tai-Wang Tai-Wang left a comment

Choose a reason for hiding this comment

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

There are two issues related to compatibility:

  • We need to upgrade the lower bound of mmcv version if merging this PR?
  • Does mmcv support the previous version of nms? I do not check the PRs in mmcv, but maybe we need to keep the deprecated version for several releases and add compatibility docs in mmcv/mmdet3d.

@filaPro
Copy link
Contributor Author

filaPro commented Apr 20, 2022

Actually this PR is not connected to any PRs to mmcv for now. We simply replace some current cuda ops from mmcv with some equal cuda ops. So this PR implies no compatibility issues of this kind.

But, if it will be merged, than we can update unused iou3d in mmcv.

@Tai-Wang
Copy link
Member

Actually this PR is not connected to any PRs to mmcv for now. We simply replace some current cuda ops from mmcv with some equal cuda ops. So this PR implies no compatibility issues of this kind.

But, if it will be merged, than we can update unused iou3d in mmcv.

Great, so the main idea is to merge nms ops in mmcv and use the unified one in mmdet3d? Then I think this PR is ready.

@ZwwWayne ZwwWayne changed the base branch from master to dev April 20, 2022 09:08
@ZwwWayne ZwwWayne changed the base branch from dev to master April 20, 2022 09:09
@ZwwWayne
Copy link
Collaborator

Hi @filaPro ,
Thanks for your kind contribution. We plan to merge this PR, could you rebase this branch to dev so that we can merge it into dev?

@filaPro filaPro changed the base branch from master to dev April 20, 2022 09:28
@CLAassistant
Copy link

CLAassistant commented Apr 20, 2022

CLA assistant check
All committers have signed the CLA.

@filaPro
Copy link
Contributor Author

filaPro commented Apr 20, 2022

Rebase somehow broke CLA agreement of the author of one of previous commits. Can we ignore it?

Hm, everything is ok now.

@ZwwWayne ZwwWayne merged commit 6dd5d32 into open-mmlab:dev Apr 20, 2022
@filaPro filaPro deleted the remove_iou3d branch April 22, 2022 14:44
@filaPro filaPro mentioned this pull request May 12, 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.

4 participants