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

[Refactor] Camera keys #805

Merged
merged 5 commits into from
Aug 6, 2021
Merged

Conversation

filaPro
Copy link
Contributor

@filaPro filaPro commented Jul 29, 2021

  1. Remove legacy keys from Collect3D: rect, Trv2c, P2.
  2. Rename cam_intrinsic to cam2img to be consistent with depth2img and lidar2img. In next PRs this can unify image dependent functions like visualization and point fusion. More discussion in #791#issuecomment-887258262.

@filaPro filaPro changed the title [Fix ] Camera keys [Refactor] Camera keys Jul 29, 2021
@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #805 (3d77518) into master (a876a47) will not change coverage.
The diff coverage is 54.54%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #805   +/-   ##
=======================================
  Coverage   49.07%   49.07%           
=======================================
  Files         210      210           
  Lines       15961    15961           
  Branches     2547     2547           
=======================================
  Hits         7833     7833           
  Misses       7626     7626           
  Partials      502      502           
Flag Coverage Δ
unittests 49.07% <54.54%> (ø)

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

Impacted Files Coverage Δ
mmdet3d/apis/inference.py 43.21% <0.00%> (ø)
mmdet3d/datasets/nuscenes_mono_dataset.py 37.08% <ø> (ø)
mmdet3d/datasets/pipelines/formating.py 60.00% <ø> (ø)
mmdet3d/models/dense_heads/fcos_mono3d_head.py 12.06% <0.00%> (ø)
mmdet3d/models/detectors/single_stage_mono3d.py 16.66% <0.00%> (ø)
mmdet3d/core/visualizer/image_vis.py 71.83% <80.00%> (ø)
mmdet3d/datasets/pipelines/loading.py 90.61% <100.00%> (ø)
mmdet3d/datasets/pipelines/transforms_3d.py 90.41% <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 a876a47...3d77518. 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.

I assume that you have used Ctrl+F to find and replace all the cam_intrinsic to cam2img? Also, please kindly test the code by browse_dataset.py on nuScene mono dataset and running the mono3d detection demo to check the visualization results. Thanks!

@filaPro
Copy link
Contributor Author

filaPro commented Jul 30, 2021

I assume that you have used Ctrl+F to find and replace all the cam_intrinsic to cam2img?

Unfortunately, no, as we have some cam_intrinsic in KITTI, nuScenes and Lyft annotations. So I replaced them only after loading pipelines. So, after Collect3D we will not have cam_intrinsic. But for visualization and browse dataset will probably keep it.

Also, please kindly test the code by browse_dataset.py on nuScene mono dataset and running the mono3d detection demo to check the visualization results.

Ok will try to visualize.

@Wuziyi616
Copy link
Contributor

I assume that you have used Ctrl+F to find and replace all the cam_intrinsic to cam2img?

Unfortunately, no, as we have some cam_intrinsic in KITTI, nuScenes and Lyft annotations. So I replaced them only after loading pipelines. So, after Collect3D we will not have cam_intrinsic. But for visualization and browse dataset will probably keep it.

Also, please kindly test the code by browse_dataset.py on nuScene mono dataset and running the mono3d detection demo to check the visualization results.

Ok will try to visualize.

That makes sense. Looking forward to the visualization results :)

@filaPro
Copy link
Contributor Author

filaPro commented Aug 2, 2021

Hi @Tai-Wang, @Wuziyi616,

Monocular demo seems to be broken and not by my commits. Am I missing something?
n015-2018-07-24-11-22-45+0800__CAM_BACK__1532402927637525_pred

@Tai-Wang
Copy link
Member

Tai-Wang commented Aug 2, 2021

Hi @filaPro , that's due to code refactoring for removing hacks in the previous codes. See #744 and #794 . The pretrained model link should be updated in the documentation (I will create a PR to fix that). You can try to pull the latest code and use the updated checkpoints. Everything seems fine from my side.

@filaPro
Copy link
Contributor Author

filaPro commented Aug 2, 2021

You can try to pull the latest code and use the updated checkpoints

Really I pulled master and downloaded the checkpoint today. Will check one more time.

UPD. Sorry for that. Master branch is fine.

@Tai-Wang
Copy link
Member

Tai-Wang commented Aug 2, 2021

You can try to pull the latest code and use the updated checkpoints

Really I pulled master and downloaded the checkpoint today. Will check one more time.

UPD. Sorry for that. Master branch is fine.

Never mind :) Thank you for reminding me the link has not been updated. It really makes that example confusing.

@filaPro
Copy link
Contributor Author

filaPro commented Aug 2, 2021

@Tai-Wang, looks like browse_dataset is also broken. Here we have not enough parameters for show_multi_modality_result call. Am I right?

As a temporal solution we can set bbox_mode='lidar' here. Do I need to make a separate PR?

BTW, have you checked this in current master? I'm getting invalid bbox rotations now. (But maybe I'm missing something again)

@Tai-Wang
Copy link
Member

Tai-Wang commented Aug 3, 2021

@filaPro That seems a legacy bug. It is not introduced by my refactoring, but I will create a PR to fix that.

After fixing the missing parameter, everything is ok for me. Have you re-generated the json files after merging the master? I have removed the hacks by adding some transformations in the pre-processing and post-processing. See more details in the compatibility documentation.

@filaPro
Copy link
Contributor Author

filaPro commented Aug 4, 2021

@Wuziyi616, @Tai-Wang browse_dataset and mono_det_demo are fine.
n015-2018-07-18-11-07-57+0800__CAM_FRONT__1531883530912460_gt
n015-2018-07-24-11-22-45+0800__CAM_BACK__1532402927637525_pred

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 visualization seems to be good. Great job!

@ZwwWayne ZwwWayne merged commit 9f0b01c into open-mmlab:master Aug 6, 2021
@filaPro filaPro deleted the fix_camera_keys branch August 6, 2021 06:24
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