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

fix bbox direction #1330

Closed
wants to merge 2 commits into from
Closed

fix bbox direction #1330

wants to merge 2 commits into from

Conversation

zhulf0804
Copy link
Contributor

@zhulf0804 zhulf0804 commented Mar 21, 2022

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

When training PointPillars, the bbox direction gt label is set to 0 when the angle is between in [0, np.pi] and is set to 1 when the angle is between in [np.pi, 2*np.pi].

offset_rot = limit_period(rot_gt - dir_offset, 0, 2 * np.pi)
dir_cls_targets = torch.floor(offset_rot / (2 * np.pi / num_bins)).long()
dir_cls_targets = torch.clamp(dir_cls_targets, min=0, max=num_bins - 1)

However, in the test stage, the operation may be opposite. That is the predicted label 0 corresponds to [-np.pi, 0] or [np.pi, 2*np.pi], while the predicted label 1 corresponds to [0, np.pi]

if bboxes.shape[0] > 0:
dir_rot = limit_period(bboxes[..., 6] - self.dir_offset,
self.dir_limit_offset, np.pi)
bboxes[..., 6] = (
dir_rot + self.dir_offset +
np.pi * dir_scores.to(bboxes.dtype))

Modification

I modified in predicted bbox prediction (mmdet3d/models/dense_heads/anchor3d_head.py#L506-L511) as the following code:

if bboxes.shape[0] > 0:
            dir_rot = limit_period(bboxes[..., 6] - self.dir_offset,
                                   self.dir_limit_offset, np.pi)
            bboxes[..., 6] = (
                dir_rot + self.dir_offset + np.pi *
                (1 - dir_scores.to(bboxes.dtype)))

BC-breaking (Optional)

Does the modification introduce changes that break the back-compatibility of the downstream repos?
If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.

Use cases (Optional)

If this PR introduces a new feature, it is better to list some use cases here, and update the documentation.

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.

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2022

CLA assistant check
All committers have signed the CLA.

@ZwwWayne
Copy link
Collaborator

Hi @zhulf0804 ,
Thanks for your kind PR. Could you fix the lint issue? You can find more details here.
image

@zhulf0804
Copy link
Contributor Author

zhulf0804 commented Mar 22, 2022 via email

@ZwwWayne ZwwWayne requested a review from Tai-Wang March 23, 2022 00:40
@Tai-Wang
Copy link
Member

This bug seems to be fixed in the latest mmdet3d. I found your used version is about v0.16.0. This bug can be fixed via setting the dir_limit_offset to 0 to keep it consistent with that in get_direction_target. You can also refer to this issue in SECOND for more details.

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 double-check whether this problem exists in the latest mmdet3d. I guess it has been fixed as I commented.

@Tai-Wang Tai-Wang added the question Further information is requested label Mar 23, 2022
@zhulf0804
Copy link
Contributor Author

Thank you for your quick reply. I checked that It has been fixed in both master and v1.0.0rc0.
However, I just used the previous version v0.18.1.

I thought I used a very new version, because v0.18.1 was released on Feb 09, 2022. I'm sorry for not checking the latest version before asking the question.

By the way, I found the bug was fixed in #803 on Jul 29, 2021.
Then It was not updated until recently in v1.0.0rc0 version 22 days ago (maybe on Mar 1, 2022.). I wonder why not considering updating it in earlier versions ?

Looking forward to your reply.

Anyway, nice repo and implementations.

@zhulf0804 zhulf0804 closed this Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants