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

ros1_node for landmark_expression_recognition updated #307

Merged
merged 20 commits into from
Oct 24, 2022
Merged

Conversation

negarhdr
Copy link
Collaborator

This branch fixes the issues of ROS1 node for landmark_based_facial_expression_recognition tool.

@tsampazk tsampazk added the test sources Run style checks label Sep 14, 2022
Copy link
Collaborator

@tsampazk tsampazk 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 for the updates to the node!

I have added some minor comments that need to be addressed before merging.

@tsampazk
Copy link
Collaborator

As a general comment, i tried to run the node even though i don't have the pretrained models. Running the node threw an error:

Running script landmark_based_facial_expression_recognition.py...
[rosrun] Couldn't find executable named landmark_based_facial_expression_recognition.py below ~/opendr/projects/opendr_ws/src/perception
[rosrun] Found the following, but they're either not files,
[rosrun] or not executable:
[rosrun]   ~/opendr/projects/opendr_ws/src/perception/scripts/landmark_based_facial_expression_recognition.py

I resolved by adding the script here, running catkin_make install and then source install/setup.bash instead of source devel/setup.bash. Then the node fails when it can't find the model as expected.

This might have revealed a bigger issue with the ROS1 package and its usage, as only some of the nodes are listed in CMakeLists.txt and the instructions read to just run catkin_make and source devel/setup.bash.

I ran into this issue today while trying to test this node, up until yesterday all nodes ran successfully by following the instructions in the readme. Nodes that ran locally yesterday (and are included in CMakeLists.txt), such as pose estimation, were throwing import errors today, because they used the local python interpreter instead of venv (shebang was ignored as explained here and here). The solution was the same (catkin_make install source install/setup.bash).

@stefaniapedrazzi, @passalis what do you think about this? Should i open an issue documenting what i found, so we can independently test it? Could it be a local issue on my machine?

@tsampazk
Copy link
Collaborator

@stefaniapedrazzi, @passalis what do you think about this? Should i open an issue documenting what i found, so we can independently test it? Could it be a local issue on my machine?

A colleague independently did a fresh install of ros and ran the nodes as documentation suggests and they ran ok. The issue seems to be on my machine, so i will do a fresh install and investigate further. Right now i can only say that the issue appeared when i checked out Negar's branch and tried to test the node, seemingly through no fault of the changes in this branch.

ad-daniel and others added 13 commits September 20, 2022 10:29
…l_expression_recognition.py

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
…l_expression_recognition.py

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
…l_expression_recognition.py

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
…l_expression_recognition.py

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
…l_expression_recognition.py

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
…l_expression_recognition.py

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
…l_expression_recognition.py

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
@negarhdr
Copy link
Collaborator Author

Please also check this branch if this is ready to be merged. Thanks.

@tsampazk
Copy link
Collaborator

Hey thanks @negarhdr, i will add a new review. Until i do, could you please take a look at the pep8 errors that fail the test?

@tsampazk tsampazk self-requested a review October 19, 2022 11:04
Copy link
Collaborator

@tsampazk tsampazk left a comment

Choose a reason for hiding this comment

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

Thanks @negarhdr! Node looks good, apart from a couple of minor things.

negarhdr and others added 2 commits October 21, 2022 21:35
…l_expression_recognition.py

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
…l_expression_recognition.py

Co-authored-by: Kostas Tsampazis <27914645+tsampazk@users.noreply.github.com>
@negarhdr
Copy link
Collaborator Author

Thanks @negarhdr! Node looks good, apart from a couple of minor things.

Thanks! The comments are addressed.

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.

Thank you

Copy link
Collaborator

@tsampazk tsampazk 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!

@negarhdr negarhdr merged commit a427fd2 into develop Oct 24, 2022
@negarhdr negarhdr deleted the fer_ros1_fix branch October 24, 2022 12:10
@tsampazk tsampazk mentioned this pull request Oct 25, 2022
@tsampazk
Copy link
Collaborator

In regards to the issue stated above, there's a relevant discussion here.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
test sources Run style checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants