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] Refine tutorials docs #666

Merged
merged 3 commits into from
Jul 1, 2021

Conversation

wHao-Wu
Copy link
Contributor

@wHao-Wu wHao-Wu commented Jun 21, 2021

  • Fix some urls in tutorials with fixed codebase version
  • Fix some typo errors in tutorials

@wHao-Wu wHao-Wu requested a review from Tai-Wang June 21, 2021 17:04
@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #666 (7dc5068) into master (b814e7f) will increase coverage by 0.04%.
The diff coverage is 64.86%.

❗ Current head 7dc5068 differs from pull request most recent head 1b6934c. Consider uploading reports for the commit 1b6934c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
+ Coverage   50.97%   51.01%   +0.04%     
==========================================
  Files         202      204       +2     
  Lines       15311    15383      +72     
  Branches     2488     2488              
==========================================
+ Hits         7805     7848      +43     
- Misses       6988     7017      +29     
  Partials      518      518              
Flag Coverage Δ
unittests 51.01% <64.86%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
mmdet3d/apis/inference.py 54.95% <0.00%> (+1.32%) ⬆️
mmdet3d/core/bbox/box_np_ops.py 26.81% <ø> (ø)
mmdet3d/core/bbox/structures/coord_3d_mode.py 61.87% <ø> (-1.26%) ⬇️
mmdet3d/datasets/pipelines/formating.py 56.00% <ø> (ø)
mmdet3d/models/detectors/imvotenet.py 28.65% <0.00%> (ø)
mmdet3d/models/detectors/imvoxelnet.py 56.36% <56.36%> (ø)
mmdet3d/models/necks/imvoxel_neck.py 66.66% <66.66%> (ø)
mmdet3d/__init__.py 81.48% <100.00%> (ø)
mmdet3d/core/bbox/structures/utils.py 97.14% <100.00%> (+0.08%) ⬆️
mmdet3d/core/visualizer/image_vis.py 71.83% <100.00%> (-2.25%) ⬇️
... and 7 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 b814e7f...1b6934c. 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.

Almost OK I think.

@@ -98,11 +98,11 @@ For each operation, we list the related dict fields that are added/updated/remov

`GlobalRotScaleTrans`
- add: pcd_trans, pcd_rotation, pcd_scale_factor
- update: points, *bbox3d_fields
- update: points, bbox3d_fields
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the * here indicate that, input_dict['bbox3d_fields'] itself is not changed, but input_dict[item] for item in bbox3d_fields will be changed. For example here in GlobalRotScaleTrans, it changes all the boxes, where the boxes key in the dict is indicated by bbox3d_fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I have also considered that case. I just wonder whether to unify the format of bbox3d_fields in data loading and pre-processing, or just to follow the reasonable pipeline in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got your point. But at least bbox3d_fields itself is not changed right? I prefer leaving *bbox3d_fields unchanged here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are definitely right. Maybe we need a further review.

Copy link
Member

@Tai-Wang Tai-Wang Jun 28, 2021

Choose a reason for hiding this comment

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

I think keeping * will be better.

@wHao-Wu wHao-Wu requested a review from Tai-Wang June 24, 2021 08:59
```

- Use `custom_imports` in the config to manually import it
or use `custom_imports` in the config to manually import it
Copy link
Member

@Tai-Wang Tai-Wang Jun 28, 2021

Choose a reason for hiding this comment

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

- Or use

@@ -248,7 +248,7 @@ __all__ = [..., 'MyHook']

```

- Use `custom_imports` in the config to manually import it
or use `custom_imports` in the config to manually import it
Copy link
Member

Choose a reason for hiding this comment

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

- Or use

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.

Just fixing the comments will be ok

@wHao-Wu wHao-Wu requested a review from ZwwWayne June 30, 2021 16:46
@ZwwWayne ZwwWayne merged commit ed090ed into open-mmlab:master Jul 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