-
Notifications
You must be signed in to change notification settings - Fork 95
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
High Resolution Pose Estimation new tool #356
Conversation
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.
Thank you, I've left a few comments. Additionally, the current PR is missing:
- a CHANGELOG entry should be added
- the demo folder should be placed inside
projects/python/perception/pose_estimation
(and lightweight_open_pose should be moved there too). - the documentation page for this tool should be referred in the index.md file
- the demo should also be referred in the index.md file (please also fix the lightweight open pose link there since it has been moved)
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
Also the pose_estimation test seems to have been canceled for taking 6+ hours. Can you check if it's a random failure or if there is indeed an issue with the changes done in this PR? (you can rerun the test 2-3 times after you've done the the fixes) |
projects/python/perception/high_resolution_pose_estimation/README.md
Outdated
Show resolved
Hide resolved
projects/python/perception/high_resolution_pose_estimation/README.md
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/algorithm/datasets/transformations.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/algorithm/datasets/coco.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/algorithm/models/with_mobilenet.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/algorithm/modules/conv.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/algorithm/scripts/make_val_subset.py
Outdated
Show resolved
Hide resolved
Hey @ad-daniel, from what i can tell, it seems to be an issue with this PR, as the regular pose estimation tests in other PRs are completed in 10-15 mins. I triggered a rerun to check again. |
Co-authored-by: ad-daniel <[email protected]> Co-authored-by: Nikolaos Passalis <[email protected]>
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.
Thanks @mthodoris! I added several comments apart from what Nikos and Daniel mentioned.
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/HighResolutionLearner.py
Outdated
Show resolved
Hide resolved
Something might indeed be wrong, the pose estimation test has been running for 1h30 after the last retry. I've canceled the job |
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 have simplified a bit the main learner class to further reduce code duplication. Please check that everything is still ok. I also think that we should have docstrings similar to the rest of the tools.
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
edited hardcoded values, removed unnecessary values from learner, added docstrings in functions edited the readme file after the changes
Co-authored-by: Nikolaos Passalis <[email protected]>
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.
Thank you! I am ok with the changes.
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.
Thanks Thodoris and Nikos for the fixes so far. Some final comments, most are minor!
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
src/opendr/perception/pose_estimation/hr_pose_estimation/high_resolution_learner.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Kostas Tsampazis <[email protected]>
Co-authored-by: Kostas Tsampazis <[email protected]>
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.
Thank you, changes LGTM!
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.
Thank you
* High Resolution Pose Estimation new tool * changes on path for 1080pi image input * adding height1 height2 as params to learner * typos * fixed tests * edit Readme files, edit files(PEP8 changes) to pass tests * fix formatting * Apply suggestions from code review Co-authored-by: ad-daniel <[email protected]> Co-authored-by: Nikolaos Passalis <[email protected]> * Minor fix in comments of original pose estimation node * HR pose estimation ros1 node * Apply suggestions from code review Co-authored-by: ad-daniel <[email protected]> Co-authored-by: Kostas Tsampazis <[email protected]> * suggestions from review (edit functions, code duplication,typos,etc) * suggestions from review (edit functions, code duplication,typos,etc) * edit some paths * changes for test errors * apply suggestions from review * Apply suggestions from code review type casting issue on height variables str() to int() Co-authored-by: Nikolaos Passalis <[email protected]> * Missing stuff * add a CHANGELOG entry, reference the demo and documentation in index.md * Simplified HR pose estimation * pep8 fixes * apply review suggestions edited hardcoded values, removed unnecessary values from learner, added docstrings in functions edited the readme file after the changes * Update docs/reference/high-resolution-pose-estimation.md Co-authored-by: Nikolaos Passalis <[email protected]> * Apply suggestions from code review Co-authored-by: Kostas Tsampazis <[email protected]> * Apply suggestions from code review Co-authored-by: Kostas Tsampazis <[email protected]> * review fixes * typos Co-authored-by: Kostas Tsampazis <[email protected]> Co-authored-by: ad-daniel <[email protected]> Co-authored-by: ad-daniel <[email protected]> Co-authored-by: Nikolaos Passalis <[email protected]> Co-authored-by: Nikolaos <[email protected]>
High-Resolution Pose Estimation tool