-
Notifications
You must be signed in to change notification settings - Fork 139
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
Add multiprocessing for eval_bop19_pose.py #110
Conversation
To test the multiprocessing:
|
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.
Please have a look at my comments.
scripts/eval_calc_errors.py
Outdated
scene_errs = [] | ||
|
||
# collect all the images and their targets | ||
im_metaDatas = [] |
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.
Please follow the snake case style, e.g., let's rename "im_metaDatas" to "im_meta_datas" or just "im_metadatas".
bop_toolkit_lib/tests/unit_test.sh
Outdated
@@ -0,0 +1,21 @@ | |||
#!/bin/bash |
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.
Please change the name of this file to reflect what it is testing (e.g. "eval_bop19_pose_test.sh").
@@ -0,0 +1,107 @@ | |||
# Author: Tomas Hodan (hodantom@cmp.felk.cvut.cz) | |||
# Center for Machine Perception, Czech Technical University in Prague | |||
|
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.
Please add a docstring at the beginning of this script to briefly describe what this script does ('''docstring''', such as in https://github.com/thodan/bop_toolkit/blob/master/scripts/calc_gt_info.py).
bop_toolkit_lib/tests/unit_test.sh
Outdated
|
||
# Loop through each file name and execute the command | ||
for FILE_NAME in "${FILE_NAMES[@]}"; do | ||
python scripts/eval_bop19_pose.py --renderer_type=vispy --results_path $INPUT_DIR --eval_path $INPUT_DIR --result_filenames=$FILE_NAME --num_worker 10 >> $OUTPUT_FILE 2>&1 |
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.
A unit test should ideally compare the output with an expected output. Would it be possible to add such a comparison (i.e. compare the content of the output files in this case)? Now it is not clear how to learn that the test run was successful (other than it didn't crash).
bop_toolkit_lib/tests/README.md
Outdated
@@ -0,0 +1,9 @@ | |||
Simple tests for using multiprocessing with the BOP toolkit. |
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 would change this line to "# Unit tests of the BOP toolkit" as the test below doesn't test just multiprocessing and we may want to add more tests in the future. I would then add a subsection "## Test of eval_bop19_pose.py" above the below command and table.
bop_toolkit_lib/renderer_batch.py
Outdated
for worker_id in range(self.num_workers): | ||
cmd = [ | ||
"python", | ||
"bop_toolkit_lib/call_renderer.py", |
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.
Is parallel calling of this call_renderer
script (which always needs to initiate the rendering buffer, load object model, etc. from scratch) really faster than just running a single renderer (that can be initialized only once at the beginning; as before this diff) in sequence? If not or if the difference is not significant (which I assume) I would stick to calculating VSD in sequence (reason: the code is simpler to understand and maintain).
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.
As shown in the table above, the run-time difference is significant for BOP5 datasets. This is because (1) the initialisation is not very long (<10s) while all the workers are run in parallel, and (2) the number of images to render is much larger than the number of objects to initialize + the renderer buffer . However, I got your points so I add this check which will make sure that the multiprocessing is done when it is need.
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.
To make the code more readable and modular, it would be great if we could clearly divide the renderer and the error computation code. For example,
- defining POSE_ERROR_VSD_ARGS in
bop_toolkit_lib/pose_error.py
- rename
call_renderer.py
tocall_vsd_worker.py
- pass the
batch_renderer
topose_error.vsd(..)
instead of other renderersren
, and then compute vsd error in that function based on the renderer argument type.
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.
In my latest commit, I updated with your reviews except for the last point regarding batch renderer. As we discussed last week, I initially attempted to do exactly what you suggested but unfortunately, none of the renderers in the toolkit allow multiprocessing. Therefore, I had to find an alternative solution by running call_vsd_worker.py
n_workers times.
…itives for benchmarking
To test To test |
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.
@nv-nguyen I'm not yet done with the review, but feel free to already address the first comments.
bop_toolkit_lib/renderer_batch.py
Outdated
for worker_id in range(self.num_workers): | ||
cmd = [ | ||
"python", | ||
"bop_toolkit_lib/call_renderer.py", |
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.
To make the code more readable and modular, it would be great if we could clearly divide the renderer and the error computation code. For example,
- defining POSE_ERROR_VSD_ARGS in
bop_toolkit_lib/pose_error.py
- rename
call_renderer.py
tocall_vsd_worker.py
- pass the
batch_renderer
topose_error.vsd(..)
instead of other renderersren
, and then compute vsd error in that function based on the renderer argument type.
scripts/eval_calc_errors.py
Outdated
if p["num_workers"] == 1: | ||
e = pose_error.vsd( | ||
R_e, | ||
t_e, | ||
R_g, | ||
t_g, | ||
depth_im, | ||
K, | ||
p["vsd_deltas"][dataset], | ||
p["vsd_taus"], | ||
p["vsd_normalized_by_diameter"], | ||
models_info[obj_id]["diameter"], | ||
ren, | ||
obj_id, | ||
"step", | ||
) | ||
else: # delayed the calculation of the error for renderer_batch | ||
e = renderer_batch.POSE_ERROR_VSD_ARGS( | ||
R_e=R_e, | ||
t_e=t_e, | ||
R_g=R_g, | ||
t_g=t_g, | ||
depth_im=depth_im, | ||
K=K, | ||
vsd_deltas=p["vsd_deltas"][dataset], | ||
vsd_taus=p["vsd_taus"], | ||
vsd_normalized_by_diameter=p[ | ||
"vsd_normalized_by_diameter" | ||
], | ||
diameter=models_info[obj_id]["diameter"], | ||
obj_id=obj_id, | ||
step="step", | ||
) | ||
|
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.
So here it would be nice to just call pose_error.vsd(..)
with either ren
or renderer_batch
and then call the vsd error computation inside.
# Define the dataset dictionary | ||
FILE_DICTIONARY = { | ||
# "icbin": "cnos-fastsammegapose_icbin-test_7c9f443f-b900-41bb-af01-09b8eddfc2c4.csv", | ||
# "tudl": "cnos-fastsammegapose_tudl-test_1328490c-bf88-46ce-a12c-a5e5a7712220.csv", | ||
"lmo": "cnos-fastsammegapose_lmo-test_16ab01bd-f020-4194-9750-d42fc7f875d2.csv", | ||
# "ycbv": "cnos-fastsammegapose_ycbv-test_8fe0af14-16e3-431a-83e7-df00e93828a6.csv", | ||
# "tless": "cnos-fastsammegapose_tless-test_94e046a0-42af-495f-8a35-11ce8ee6f217.csv", | ||
# Add more entries as needed | ||
} |
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.
Could we maybe run a last sanity test and check whether we get 0mAP with empty predictions and 100mAP with ground truth predictions?
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.
Yes, I agree this is important. I added in my latest commit the sanity test with ground-truth predictions.
For empty predictions, maybe you mean “totally wrong” predictions instead of “empty” since the current code will already raise some errors when the predictions are empty anyway.
scripts/eval_calc_errors.py
Outdated
"step", | ||
) | ||
else: # delayed the calculation of the error for renderer_batch | ||
e = renderer_batch.POSE_ERROR_VSD_ARGS( |
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.
e = renderer_batch.POSE_ERROR_VSD_ARGS( | |
e = pose_error.POSE_ERROR_VSD_ARGS( |
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.
Updated. Thanks!
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.
lgtm
Adding multiprocessing with 10 workers by default for evaluation script eval_bop19_pose.py. The original script can still be run with argument
--num_worker 1
.I made some unit tests with MegaPose's results.
Here are the run-time comparisons with and without multiprocessing. Note that the scores remains unchanged. The run-time improvement is more significant for large datasets such as T-LESS.
I can do some refactoring to make the code cleaner after this pull-request is accepted.
@thodan @MartinSmeyer @ylabbe