-
Notifications
You must be signed in to change notification settings - Fork 133
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
Qdtrack #183
Qdtrack #183
Conversation
@mageofboy Is this PR ready for review? |
Sorry for the delayed response. Yes, it is mostly ready, outside of some reformatting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking good, but please address my comments before we merge in the PR.
configs/tracking.conf
Outdated
@@ -15,7 +15,7 @@ | |||
--simulator_num_people=200 | |||
--simulator_num_vehicles=10 | |||
######### Other config ######### | |||
--visualize_tracked_obstacles | |||
# --visualize_tracked_obstacles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please revert changes to this config file. I think it's best if this config file uses sort by default because it's a well tested tracker that doesn't have many dependencies.
pylot/perception/flags.py
Outdated
'Path to the model') | ||
flags.DEFINE_string( | ||
'qd_track_config_path', | ||
'dependencies/qdtrack/configs/qdtrack-frcnn_r50_fpn_12e_bdd100k.py', #FIX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a fix comment?
flags.DEFINE_string( | ||
'qd_track_config_path', | ||
'dependencies/qdtrack/configs/qdtrack-frcnn_r50_fpn_12e_bdd100k.py', #FIX | ||
'Path to the model') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the Path to the model configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, it's the qd track model path but it's currently the path to the bdd model, rather than the waymo model
git clone https://github.com/mageofboy/qdtrack.git | ||
git clone https://github.com/open-mmlab/mmdetection.git | ||
cd mmdetection | ||
python3 setup.py develop #need to add mmcv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How reliable is mmdetection's installation? Did it work out of the box for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately it is not reliable and installation depends on torch and cuda version
Also, can you please make the code style changes necessary to ensure that the CI passes? You can see the suggested changes here: https://github.com/erdos-project/pylot/pull/183/checks?check_run_id=2488080643 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! There's only one comment left to address.
No description provided.