Add Autoware compatibility #1
Conversation
|
@kaancolak Apologies for the late review 🙇 |
|
Thank you @yukke42 -san, I shared over google drive: |
|
@kaancolak |
|
Hi @yukke42 -san, I am so sorry for the delay, I was working on another project. 🙇 Before the implementation, I asked for suggestions on how to implement it. But I completely agree with you, the formal project style is simpler and easier to track. Subfolders of mmdet3d could be implemented with the project style as inherited from the main repo. But for tools, probably we need to hard copy to the original repository. If you want to go with this way, I can convert the repository to the official project style. |
@kaancolak In that case, it would be simpler and easier to manage codes by creating the project style folder in this repository. |
|
@yukke42 -san, thank you, I just want to confirm.
|
Signed-off-by: Kaan Çolak <[email protected]>
kaancolak
force-pushed
the
dev-1.x-autoware
branch
from
5bd5365 to
ff616ff
Compare
|
@yukke42 -san, friendly ping. 🙇 I updated the repos, could you check and review it? |
Signed-off-by: Kaan Çolak <[email protected]>
|
@kaancolak |
Shin-kyoto
requested review from
Shin-kyoto and
kminoda
and removed request for
yukke42
|
Your PR has many areas that can be improved by applying pre-commit (for example, missing a newline at the end of the file, etc.). Could you add CI to apply this automatically? |
| self.z_offset = self.vz / 2 + point_cloud_range[2] | ||
| self.point_cloud_range = point_cloud_range | ||
|
|
||
| def forward(self, features: Tensor, num_points: Tensor, coors: Tensor, |
There was a problem hiding this comment.
@kaancolak
This function is very long, and it is really difficult for reviewer to check. So could you cover this function by test?
There was a problem hiding this comment.
Actually, this function is hard copied from the original repository, it just contains a small adjustment to make it compatible with Autoware's centerpoint TensorRT implementation. Autoware's implementation doesn't contain voxel center z as an encoder input but the original mmdetection3d implementation contains it.
There was a problem hiding this comment.
Thank you for the quick response! This repository is open-source and will receive PRs from various contributors. To ease the burden on reviewers, adding tests seems to be the simplest solution for the future. What do you think? It's also fine to include tests that are already present in the original code.
There was a problem hiding this comment.
@Shin-kyoto -san, thank you.
IMO, we can add tests for customized operations like "autoware_voxel_encoder", and "T4Dataset" operations, etc. , original parts of the implementations have already been checked with the test under mmdet main repository.
There was a problem hiding this comment.
I also think that we can add tests for customized operations. PillarFeatureNetAutoware is also customized class and test will be really helpful when we review the PR.
There was a problem hiding this comment.
Hi @Shin-kyoto -san, I added tests for the T4 dataset and autoware_voxel_encoder, PillarFeatureNetAutoware base class of autoware_voxel_encoder.
You can basically run with
cd mmdetection3d/ && pytest -s projects/AutowareCenterPoint/tests/
Relevant files:
https://github.com/autowarefoundation/mmdetection3d/pull/1/files#diff-0b5c1ac645064857b8ac247c5d193291b74c6130bfdfa4c22aed90d2bd0df58a
Co-authored-by: Shintaro Tomie <[email protected]>
|
@Shin-kyoto - san, thank you so much for your quick review and advice. I will apply an automatic CI check to a repository. |
|
@kaancolak Hi, as reviewed above, currently the repository does not work due to this commit: 37fc84d. Given that the contract between us was till the end of this month, please prioritize to make the whole pipeline work as intended as soon as possible so that we can review at least from the user's perspective. |
Signed-off-by: Kaan Çolak <[email protected]>
Signed-off-by: Kaan Çolak <[email protected]>
Signed-off-by: Kaan Çolak <[email protected]>
|
Hi @kminoda @Shin-kyoto , I apologize for not addressing this matter earlier as I was dealing with other issues. I have made updates to the CI actions and changed the dataset name to "T4Dataset." It is now prepared for review. |
Signed-off-by: Kaan Colak <[email protected]>
kaancolak
force-pushed
the
dev-1.x-autoware
branch
from
2ee6f33 to
d167d59
Compare
Signed-off-by: Kaan Çolak <[email protected]>
xmfcx
changed the title
|
@Shin-kyoto -san, could you review this again? |
|
I talked with AutoCore and @beginningfan could follow the instructions and see if they can retrain the model. |
| nusc, train_scenes, val_scenes, True, max_sweeps=max_sweeps) | ||
| return train_nusc_infos | ||
|
|
||
| dataset_config = root_path + version + '.yaml' |
There was a problem hiding this comment.
The version of dataset_config here does not match the T4Dataset. I downloaded the dataset here, and the yaml file inside does not have the version data passed in. Should we change the yaml file name of the dataset?
There was a problem hiding this comment.
@beginningfan with which command did you get this error? T4Dataset is the dataset format, and sample_dataset is a version of it in there. So sorry for the late response, I missed notification.
There was a problem hiding this comment.
@kaancolak Oh, my fault. I forgot to click on review changes before, which caused the comment to remain in a pending state。
I used default version param, which is v1.0. It will be ok when using sample_dataset for the version. Then I suggest writing the T4Dataset version in the document, or modifying the default parameters of the T4Dataset version number.
Thank you so much @beginningfan, do you have any plans for reviewing documentation? |
@kaancolak I've read the documentations before when testing, and it's basically fine. I still recommend adding the version of T4Dataset in the documentation to avoid confusion. You can add it to the document of this PR or your other PRs. |
I added an explanation to the training documentation. |
Changes
Blocks merging of: