-
Notifications
You must be signed in to change notification settings - Fork 29
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
Online learning dev #5
base: indigo_dev
Are you sure you want to change the base?
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.
Please update your code according to the comments, then I will check it again. And please use the nice drawing for documenting your code on ros wiki.
@@ -21,7 +21,7 @@ faces_min_search_scale_y: 30 | |||
# when this flag is activated, the 3d size of a face is determined and only reasonable sizes may continue in the processing pipeline | |||
# moreover, if activated, the background behind the head is cleared in the color image resulting in more robust recognition performance | |||
# bool | |||
reason_about_3dface_size: true | |||
reason_about_3dface_size: false |
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.
Did you turn this off on purpose? Why? If not, turn it on again.
|
||
# openface folder | ||
# string | ||
openface_directory: "/home/cag/openface/openface/" |
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.
Better use "~/openface/openface" . If you only read from there and never write, you can drop this parameter and use rospack to find the path because you need to wrap openface in a ROS package anyways (see comment at your install script).
for Care-O-bot default is cam3d_nodelet_manager --> | ||
<arg name="start_manager" default="false"/> <!-- if you do not like to use the nodelet manager provided by the openni driver, specify your own by setting this parameter to true and providing the name of your manager with argument nodelet_manager--> | ||
<arg name="display_results_with_image_view" default="true"/> <!-- set to false if you do not like to display the camera image with names attached to detected faces within an image_view window --> | ||
<arg name="recognition_method" default="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.
We need some comment here what the different options are and what they mean, e.g. <!-- specifies recognition algorithm: 0 = Fisherfaces, 1 = Eigenfaces, 2 = LDA2D, 3 = PCA2D, 4 = Deep learning based online recognition (attention: choices 0 and 1 work with a single person in the training data, options 2 and 3 will cause a crash of the program if you do not train at least 2 different people) -->
Make 2 as default option as it was initially. 1=Eigenfaces performs badly.
<?xml version="1.0"?> | ||
|
||
<launch> | ||
<arg name="camera_namespace" default=""/> |
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.
add a default value like "camera"
<launch> | ||
<arg name="camera_namespace" default=""/> | ||
<arg name="in_topic_name" value="/face_detector/face_positions"/> | ||
<arg name="recognition_method" /> |
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 repeat the comment about the different options here and set a default value, e.g. 2.
<!-- specifies recognition algorithm: 0 = Fisherfaces, 1 = Eigenfaces, 2 = LDA2D, 3 = PCA2D, 4 = Deep learning based online recognition (attention: choices 0 and 1 work with a single person in the training data, options 2 and 3 will cause a crash of the program if you do not train at least 2 different people) -->
<?xml version="1.0"?> | ||
|
||
<launch> | ||
<arg name="camera_namespace" default=""/> |
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 this necessary? Remove it if not, it does not make sense to me.
<node name="face_detector_hog" pkg="cob_people_detection" type="face_detector_hog_node.py" output="screen"> | ||
<rosparam command="load" file="$(find cob_people_detection)/ros/launch/face_detector_params.yaml"/> | ||
<param name="data_directory" type="string" value="$(find cob_people_detection)/common/files/"/> | ||
<remap from="~head_positions" to="$(arg camera_namespace)$(arg in_topic_name)"/> |
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.
Are you sure to use $(arg camera_namespace)
in front of the topic?
@@ -1,10 +1,12 @@ | |||
<?xml version="1.0"?> | |||
|
|||
<launch> | |||
<arg name="recognition_method" /> |
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.
We need some explanatory comment here, e.g.
<!-- #specifies face recognition algorithm: 0 = Fisherfaces, 1 = Eigenfaces, 2 = LDA2D, 3 = PCA2D, 4 = Deep learning based online recognition (attention: choices 0 and 1 work with a single person in the training data, options 2 and 3 will cause a crash of the program if you do not train at least 2 different people)-->
Also set a default value, e.g. 2.
<!-- learn and recognizes faces online and publish their positions --> | ||
<node name="online_face_recognizer" pkg="cob_people_detection" type="online_face_recognition_node.py" output="screen"> | ||
<rosparam command="load" file="$(find cob_people_detection)/ros/launch/online_face_recognizer_params.yaml"/> | ||
<remap from="~face_detector/face_positions" to="$(arg camera_namespace)$(arg in_topic_name)"/> |
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 seems like your pull request does not affect the camera_namespace argument. Just remove it or make a systematic proposal in all files. But for that start a new pull request.
<?xml version="1.0"?> | ||
|
||
<launch> | ||
<arg name="camera_namespace" default=""/> |
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.
remove
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 so far, only a few simple requests.
|
||
<!-- remaining launch files --> | ||
<param name="data_storage_directory" type="string" value="$(env HOME)/.ros/cob_people_detection/files/"/> | ||
<param name="face_detector_method" type="int" value="$(arg face_detector_method)"/> <!-- 0 -> hog: Histogram of Oriented Gradients, |
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.
Do you really need to create this parameter here? Is it used from some program later on?
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.
No, thanks! I missed it which is not useful anymore.
|
||
<!--Face detector--> | ||
<group if="$(arg launch_face_detector)"> | ||
<include if="$(arg face_detector_method)" file="$(find cob_people_detection)/ros/launch/face_detector_hog.launch"/> |
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.
Did you test this? I am always confused with if/unless so is 0=HOG and 1=Viola Jones as commented above?
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.
Yeah, you are right. I missed it really bad 👎 Sorry for that. I will reverse the order of if/unless.
@@ -1,17 +1,21 @@ | |||
<?xml version="1.0"?> | |||
|
|||
<launch> | |||
|
|||
<arg name="display_results_with_image_view" default="true"/> <!-- set to false if you do not like to display the camera image with names attached to detected faces within an image_view window --> | |||
<arg name="face_positions_topic_name_in" value="/face_detector/face_positions"/> |
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.
Did you test this? Does it work with "/" at the beginning? Before it was without.
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 is working with both options, but I returned to its original.
</group> | ||
|
||
<!-- remaining launch files --> | ||
<param name="data_storage_directory" type="string" value="$(env HOME)/.ros/cob_people_detection/files/"/> | ||
<group> |
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.
This should be part of above's group <group if="$(arg launch_face_recognizer)">
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.
Sure!
I pushed some commits according to your comments, Also opened a PR in cob_extern repo. |
Looks good. I am going to test the software now. By the way, did you try whether all dependencies can be installed? Like this:
I am receiving the following error:
|
I am testing it now.
…On Tue, Aug 8, 2017 at 11:37 AM, Richard Bormann ***@***.***> wrote:
Looks good. I am going to test the software now.
By the way, did you try whether all dependencies can be installed? Like
this:
rosdep install --from-paths src/ -y -i
I am receiving the following error:
ERROR: the following packages/stacks could not have their rosdep keys resolved
to system dependencies:
cob_people_detection: Cannot locate rosdep definition for [libopenface]
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG26LJEI6tY3dDlj0ugnqmJu2QWjvj5Vks5sWCxWgaJpZM4OmT8x>
.
|
Sorry, this is connected to the missing pull request to cob_extern. Please finish that one first, then we can proceed here. |
Hi,
I added some new functionalities to the repo all of them written in Python such as:
Flowchart:
After the installations, please set the correct paths of openface and model directories inside the face_detector_params.yaml and online_face_recognizer_params.yaml. If you suggest a automatized way to do this, I would really appreciate that.
TODO
This is my first pull-request for this repo. So, I appreciate your valuable comments. Thanks.
ipa-cgo (cagatay odabasi)