Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fer va estimation #264

Merged
merged 147 commits into from
Dec 21, 2022
Merged

Fer va estimation #264

merged 147 commits into from
Dec 21, 2022

Conversation

negarhdr
Copy link
Collaborator

This PR adds image-based facial expression recognition and valence-arousal estimation.
It includes learner, unit-tests, demo, document, and ROS node.
This is a replacement for the previous PR which had conflicts with other tools.

@negarhdr negarhdr added test sources Run style checks test tools Test the toolkit methods labels Jun 14, 2022
@ad-daniel
Copy link
Collaborator

Hi, maybe there was a misunderstanding. It was correct for you to create your branch from the develop (since new additions like this should target develop, not master). The issue with your previous attempt likely was that when you created the branch, develop was not up to date or you committed unrelated stuff. In short you should:

git checkout develop
git pull origin develop
git submodule update

do a git status to ensure everything looks clean, there should not be any change showing up

git checkout -b my-fancy-new-branch
do the changes and push

@negarhdr negarhdr changed the base branch from master to develop June 20, 2022 16:10
@negarhdr
Copy link
Collaborator Author

Thank you for the comment.
I found some other issues in the previous branch so I deleted it and made this new branch and the new PR.
My branch is based on "develop" branch and now its merge target is also the "develop" branch.

Copy link
Collaborator

@passalis passalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I have added a few comments directly on files.

Furthermore, there are few additional points:

  • Please make sure that you use the conventions for ROS argument passing (check also the names of the topics and arguments in ROS1 updates and fixes for several nodes #281)
  • Should this file be empty src/opendr/perception/facial_expression_recognition/image_based_facial_emotion_estimation/README.md ?
  • The demo is nice. However, for most other tools for inference demo, we provide small examples of how each tool can be used. This demo incorporates too much functionality (which is good, but it can be also hard for new users to follow). So I would propose to also include a small inference demo that runs on a single image and clearly imports and uses the learner (similar to other tools, e.g., https://github.com/opendr-eu/opendr/blob/master/projects/perception/lightweight_open_pose/demos/inference_demo.py). In this way it would be clear for new users how they should use the tool.

@negarhdr
Copy link
Collaborator Author

Thank you! I have added a few comments directly on files.

Furthermore, there are few additional points:

  • Please make sure that you use the conventions for ROS argument passing (check also the names of the topics and arguments in ROS1 updates and fixes for several nodes #281)
  • Should this file be empty src/opendr/perception/facial_expression_recognition/image_based_facial_emotion_estimation/README.md ?
  • The demo is nice. However, for most other tools for inference demo, we provide small examples of how each tool can be used. This demo incorporates too much functionality (which is good, but it can be also hard for new users to follow). So I would propose to also include a small inference demo that runs on a single image and clearly imports and uses the learner (similar to other tools, e.g., https://github.com/opendr-eu/opendr/blob/master/projects/perception/lightweight_open_pose/demos/inference_demo.py). In this way it would be clear for new users how they should use the tool.

Thanks for the comments.
The ros1_node is updated and the demo is simplified as much as possible.

@tsampazk
Copy link
Collaborator

@ad-daniel @negarhdr The ROS1 node now uses retina face, as the cv face detector proved problematic with the conversions required for ROS. I also included the publishing of an annotated image which draws bounding boxes with all facial emotion info printed on the corner of the box. I tested it with multiple faces and it works well. It is similar to the face recognition pipeline in the corresponding node. Note that it is quite slow, but the detector is far superior in the performed qualitative testing.

ad-daniel
ad-daniel previously approved these changes Dec 21, 2022
Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running the ROS node I'm getting this:

rosrun opendr_perception facial_emotion_estimation_node.py
[rospack] Error: package 'opendr_perception' not found
find: ‘’: No such file or directory
Tokenization took: 0.01 ms

however it doesn't prevent it from working, so not a big deal.

As for the demo, I've tested the webcam and it works nicely. Image and video are failing, from the error it seems to be related to the fact it cannot find the resources in the FTP server. @Pavlos-Tosidis has it been moved from owncloud? I think we forgot to ping him

Other than that, it looks good to me, thank you both!

@tsampazk
Copy link
Collaborator

When running the ROS node I'm getting this:

rosrun opendr_perception facial_emotion_estimation_node.py
[rospack] Error: package 'opendr_perception' not found
find: ‘’: No such file or directory
Tokenization took: 0.01 ms

Hmm that's weird, never gotten this error before. Maybe this helps?

Thanks!

@ad-daniel
Copy link
Collaborator

Nevermind my bad, I did a source devel/local_setup.bash rather than source devel/setup.bash.

@Pavlos-Tosidis
Copy link
Collaborator

Pavlos-Tosidis commented Dec 21, 2022

@ad-daniel
Truth is I never saw this comment for moving the demo on the FTP. Files are up now!

@tsampazk
Copy link
Collaborator

Went ahead and resolved the issue with the path in download. Also fixed the demo for image and video modes, as the cv2 operations to show the images were used incorrectly. Should be good to go now!

tsampazk
tsampazk previously approved these changes Dec 21, 2022
ad-daniel
ad-daniel previously approved these changes Dec 21, 2022
Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working for me as well, thank you!

@tsampazk tsampazk dismissed stale reviews from ad-daniel and themself via 4561c8d December 21, 2022 16:48
@tsampazk
Copy link
Collaborator

Sorry Daniel just merged hr pose estimation and there was a conflict.

@ad-daniel ad-daniel merged commit a2b1a85 into develop Dec 21, 2022
@ad-daniel ad-daniel deleted the fer_va_estimation branch December 21, 2022 18:34
@tsampazk tsampazk mentioned this pull request Dec 21, 2022
24 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
test sources Run style checks test tools Test the toolkit methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants