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

[Feature] Support DGCNN #801

Closed
wants to merge 27 commits into from
Closed

[Feature] Support DGCNN #801

wants to merge 27 commits into from

Conversation

DCNSW
Copy link
Contributor

@DCNSW DCNSW commented Jul 28, 2021

Motivation

Support DGCNN (Dynamic Graph CNN for Learning on Point Clouds) in mmdetection3d.

Modification

Design new configs, backbones, decode_heads and ops for DGCNN.

BC-breaking (Optional)

Does the modification introduce changes that break the back-compatibility of the downstream repos?
No.

Checklist

  1. Pre-commit or other linting tools are used to fix the potential lint issues.
  2. The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  3. If the modification has potential influence on downstream projects, this PR should be tested with downstream projects.
  4. The documentation has been modified accordingly, like docstring or example tutorials.

@ZwwWayne ZwwWayne requested a review from Wuziyi616 July 28, 2021 08:39
@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #801 (18541d9) into master (cab70db) will increase coverage by 0.14%.
The diff coverage is 63.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #801      +/-   ##
==========================================
+ Coverage   49.32%   49.47%   +0.14%     
==========================================
  Files         210      216       +6     
  Lines       16019    16179     +160     
  Branches     2556     2574      +18     
==========================================
+ Hits         7902     8004     +102     
- Misses       7614     7670      +56     
- Partials      503      505       +2     
Flag Coverage Δ
unittests 49.47% <63.52%> (+0.14%) ⬆️

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

Impacted Files Coverage Δ
mmdet3d/ops/dgcnn_modules/dgcnn_fa_module.py 52.17% <52.17%> (ø)
mmdet3d/ops/dgcnn_modules/dgcnn_gf_module.py 55.93% <55.93%> (ø)
mmdet3d/models/decode_heads/dgcnn_head.py 60.00% <60.00%> (ø)
mmdet3d/ops/dgcnn_modules/dgcnn_fp_module.py 64.70% <64.70%> (ø)
mmdet3d/models/backbones/dgcnn.py 78.12% <78.12%> (ø)
mmdet3d/models/backbones/__init__.py 100.00% <100.00%> (ø)
mmdet3d/models/decode_heads/__init__.py 100.00% <100.00%> (ø)
mmdet3d/ops/__init__.py 100.00% <100.00%> (ø)
mmdet3d/ops/dgcnn_modules/__init__.py 100.00% <100.00%> (ø)
mmdet3d/models/fusion_layers/point_fusion.py 64.44% <0.00%> (-0.78%) ⬇️
... and 10 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 cab70db...18541d9. Read the comment docs.

Copy link
Contributor

@Wuziyi616 Wuziyi616 left a comment

Choose a reason for hiding this comment

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

The code needs to be refined. Benchmark is required. I think the design of DGCNN is acceptable (via ops/dgcnn_modules), but we need to polish the code. Also, unit tests are needed. See my comments above.

configs/dgcnn/README.md Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_fa_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_fa_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_fa_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_fa_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_fp_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_fa_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_fp_module.py Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_gf_module.py Outdated Show resolved Hide resolved
@Wuziyi616 Wuziyi616 changed the title [Enhance] Support DGCNN [Feature] Support DGCNN Jul 30, 2021
Copy link
Contributor

@Wuziyi616 Wuziyi616 left a comment

Choose a reason for hiding this comment

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

Let's wait for results on 6-fold evaluation.

configs/dgcnn/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Wuziyi616 Wuziyi616 left a comment

Choose a reason for hiding this comment

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

@ZwwWayne Please have a look here. The benchmark is done on S3DIS (both Area 5 and 6-Fold). The code in each added module is good. However, I don't know if you think the overall design of DGCNN is acceptable (I think it's okay). Similar to PointNet++, in this repo, DGCNN is decomposed to several sub-modules, including GraphFeature (gf), FeatureAggregation (fa) and FeaturePropagation (fp). Please kindly have a look.

@ZwwWayne ZwwWayne requested a review from wHao-Wu August 4, 2021 09:57
mmdet3d/ops/dgcnn_modules/dgcnn_gf_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_gf_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_gf_module.py Outdated Show resolved Hide resolved
@Wuziyi616
Copy link
Contributor

@DCNSW Please kindly check the docstring of the code in this PR. Add optional to those args which has default values.

Copy link
Contributor

@Wuziyi616 Wuziyi616 left a comment

Choose a reason for hiding this comment

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

I think this PR is almost ready

mmdet3d/models/backbones/dgcnn.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/builder.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_gf_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_gf_module.py Outdated Show resolved Hide resolved
mmdet3d/ops/dgcnn_modules/dgcnn_gf_module.py Outdated Show resolved Hide resolved
@Wuziyi616
Copy link
Contributor

After all the comments are resolved, please use this script to gather trained model weight and zip it and send it to @ZwwWayne.

Copy link
Contributor

@Wuziyi616 Wuziyi616 left a comment

Choose a reason for hiding this comment

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

Great job. Cheers!

@ZwwWayne
Copy link
Collaborator

Considering reduce the registry of fa/fg module

@Wuziyi616
Copy link
Contributor

Wuziyi616 commented Aug 18, 2021

Some benchmark release related modifications are missed. Please refer to #541 for more details, e.g. adding DGCNN model in docs/model_zoo.md, README.md and README_zh-CN.md files

README_zh-CN.md Outdated Show resolved Hide resolved
mmdet3d/models/backbones/dgcnn.py Outdated Show resolved Hide resolved
@ZwwWayne ZwwWayne requested a review from Tai-Wang August 25, 2021 09:36
configs/dgcnn/README.md Outdated Show resolved Hide resolved
configs/dgcnn/README.md Outdated Show resolved Hide resolved
configs/dgcnn/metafile.yml Outdated Show resolved Hide resolved
mmdet3d/models/backbones/dgcnn.py Outdated Show resolved Hide resolved
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.

Please also fix minor problems similar to our comments

@DCNSW DCNSW changed the base branch from master to v1.0.0.dev0 August 31, 2021 06:33
@DCNSW DCNSW changed the base branch from v1.0.0.dev0 to master August 31, 2021 06:44
@DCNSW DCNSW closed this Sep 1, 2021
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.

5 participants