[Enhance] Add MultiViewPipeline #748
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 1.0 #748 +/- ##
==========================================
+ Coverage 48.96% 49.10% +0.14%
==========================================
Files 208 208
Lines 15880 15919 +39
Branches 2538 2546 +8
==========================================
+ Hits 7775 7817 +42
+ Misses 7603 7601 -2
+ Partials 502 501 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Of course. Just add them if necessary and make them smaller. |
|
Hi, @Tai-Wang |
|
|
||
| @PIPELINES.register_module() | ||
| class MultiViewPipeline(object): | ||
| """Load and transform multi-view images. |
There was a problem hiding this comment.
If we will put Load into transforms, a comment is needed here or somewhere else, because it seems a little inconsistent with the name transforms itself. Or do you think is it will be better to move load out of the transform like MultiScaleFlipAug3D?
There was a problem hiding this comment.
Are you suggesting to split my MultiViewPipeline to 2 separate classesMultiViewPipeline and LoadMultiViewImageFromFilesV2? Not sure that adding 2 new transforms is better then 1. But if you think it is better, I can do it.
There was a problem hiding this comment.
Also we can move to_float32, color_type, and file_client_args from LoadImageFromFile.__init__ to MultiViewPipeline.__init__ and remove LoadImageFromFile from MultiViewPipeline.transforms. However, it is not beautiful too.
I prefer to add a comment and assert about the necessity of LoadImageFromFile in MultiViewPipeline.transforms.
There was a problem hiding this comment.
@ZwwWayne Please have a look at this function and see whether there is a conflict with your multi-modality models on nuScenes.
Great job! There is only something about design details that can be further discussed (see the comments). |
|
@ZwwWayne can you please have a look here? |
More discussion in #696#issuecomment-874275857.
It's WIP now as we also need a unit test and probably ScanNet +
MultiViewPipelinetest. Can I add 2-3 ScanNet .jpg images totests/datafor this purpose? сс @Tai-WangAlso in case of ScanNet +
MultiViewPipelinetest this PR should be merged after #696.