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 ImVoxelNet on KITTI #627

Merged
merged 10 commits into from
Jun 16, 2021
Merged

Conversation

filaPro
Copy link
Contributor

@filaPro filaPro commented Jun 9, 2021

Reimplementation of ImVoxelNet for kitti-3d-car

@filaPro filaPro mentioned this pull request Jun 9, 2021
@Tai-Wang Tai-Wang changed the title ImVoxelNet [Feature] Support ImVoxelNet on KITTI Jun 9, 2021
@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #627 (9a40084) into master (c33d4ec) will increase coverage by 0.05%.
The diff coverage is 61.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #627      +/-   ##
==========================================
+ Coverage   51.04%   51.10%   +0.05%     
==========================================
  Files         201      203       +2     
  Lines       15221    15307      +86     
  Branches     2468     2472       +4     
==========================================
+ Hits         7770     7822      +52     
- Misses       6933     6967      +34     
  Partials      518      518              
Flag Coverage Δ
unittests 51.10% <61.17%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
mmdet3d/models/detectors/imvoxelnet.py 56.36% <56.36%> (ø)
mmdet3d/models/necks/imvoxel_neck.py 66.66% <66.66%> (ø)
mmdet3d/models/detectors/__init__.py 100.00% <100.00%> (ø)
mmdet3d/models/necks/__init__.py 100.00% <100.00%> (ø)
mmdet3d/datasets/lyft_dataset.py 71.35% <0.00%> (-0.22%) ⬇️
mmdet3d/core/bbox/structures/utils.py 97.05% <0.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 c33d4ec...9a40084. Read the comment docs.

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.

Overall looks good to me. Just need to fix some minor comments.
In addition, maybe we can inherit the kitti base dataset config in the imvoxelnet config?

@filaPro
Copy link
Contributor Author

filaPro commented Jun 9, 2021

In addition, maybe we can inherit the kitti base dataset config in the imvoxelnet config?

I think there are too much point-cloud related transforms and we need only images.

Also, do I need to add a unit test?

@filaPro filaPro marked this pull request as draft June 9, 2021 11:23
@Tai-Wang
Copy link
Member

Tai-Wang commented Jun 9, 2021

In addition, maybe we can inherit the kitti base dataset config in the imvoxelnet config?

I think there are too much point-cloud related transforms and we need only images.

Also, do I need to add a unit test?

OK, then we can keep the current format for configs.
Unit tests are needed, we'd better also add them in this PR.
Besides, I observe that the linting failed. You can refer to this guidance for useful pre-commit tools to avoid this problem.

@filaPro filaPro marked this pull request as ready for review June 9, 2021 13:58
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.

Almost ready to merge except consistent expression of Tensor in the docstring

@filaPro
Copy link
Contributor Author

filaPro commented Jun 9, 2021

I will also run training on 8 GPUs one more time to be sure that refactoring Anchor3DGenerator and ConvModule not much influenced anything.

@filaPro
Copy link
Contributor Author

filaPro commented Jun 11, 2021

I reverted anchor ranges, metrics are fine now. Can share model and log if needed.

@ZwwWayne ZwwWayne merged commit c1f6bba into open-mmlab:master Jun 16, 2021
@filaPro filaPro deleted the imvoxelnet branch June 17, 2021 07:30
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