Skip to content
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

[Draft] Delete single recognized face #280

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[Draft] Delete single recognized face #280

wants to merge 5 commits into from

Conversation

DerOetzi
Copy link

@DerOetzi DerOetzi commented Jun 5, 2020

This is a draft for my feature request on #158 to removed single tagged faces from database.

  • RestAPI Endpoint DELETE /face/{id}
  • Integration UI Personal Site
  • Integration UI Filebrowser PersonsTab

Signed-off-by: Johannes DerOetzi Ott info@johannes-ott.net

Johannes DerOetzi Ott added 4 commits June 5, 2020 12:37
Signed-off-by: Johannes DerOetzi Ott <info@johannes-ott.net>
Signed-off-by: Johannes DerOetzi Ott <info@johannes-ott.net>
Signed-off-by: Johannes DerOetzi Ott <info@johannes-ott.net>
Signed-off-by: Johannes DerOetzi Ott <info@johannes-ott.net>
@matiasdelellis
Copy link
Owner

Wow @DerOetzi
I see you took the work seriously. 😬
I'm not sure about this yet, but I'm going to guide you to do it right. 😉

I explain here why I said a "small hack". 😉

Remove recognized faces single and bulk.

It is tempting to delete the face as you are doing, but keep in mind that a face can be an entire cluster. You can leave a person wihtout faces, which breaks the integrity of the database. You must control this situation.

You can take ideas to improve in code of delete files.

Requeue single picture for scanning, if I remove face accidentaly.

I showed you the code of deleting a photo, but it is not the correct approach. Obtaining this information is very expensive, and therefore you should not delete it unnecessarily. You must find a way to invalidate it.

Maybe add a state column to the faces table, where 0 is ok, and 1 is 'unknown person`, etc. (I didn't think much about names or values!) and adjust the queries to this parameter.

@stalker314314 if you want to comment welcome.

Requeue single picture for scanning

This in particular is something I would like, (Especially to use when improve the server and you can use more processing power), but it is not done even in the medium term. 😅

@matiasdelellis
Copy link
Owner

Integration UI Personal Site

For now don't worry about this. This view was developed to evaluate the quality of the clusters, but the main view should be the files and photos applications.

When we develop the people view in photos, this view will be removed. 😉
If the user wants to delete/invalidate/etc a face that opens the image and does it in the sidebar.

I already said it many times. But we not pretend to be better than google .. 😅

imagen

They by default show the most important faces, and allow you to add or remove another faces. but never eliminate this.. 😉

Signed-off-by: Johannes DerOetzi Ott <info@johannes-ott.net>
@stalker314314
Copy link
Collaborator

Hi @DerOetzi - nice code and thanks for doing this! However, I don't think this is best approach (IMHO)! Output of face recognition and chinese whispers (clustering logic) is rather unknown and unpredictable and we tend to try to keep it "as-is" and only operate on higher levels. I would rather see additional table, on top of current faces, that can be consulted to tell if face should be removed, moved to another person... If this looks like overkill, then @matiasdelellis approach with new state column also should work. In other words, I would rather let user fiddle with "presentation" layer, than with output of ML models. I think @matiasdelellis even fiddled with adding something like state column in #262, maybe you should look at that. I would not mind adding new state column in Faces (but we will still need additional table, or another column in Faces to "move" face from one person/cluster to another). Current solution, while I admire your positive attitude, is maybe hackish? Hack that work, I admit!:), but looks hackish to me. What do toy think @DerOetzi on this approach (with new table, or column)?

Another, unrelated note: what if pictures changes slightly and we need to re-run face recognition - that would be completely other face? Or user wipes everything and recreates from scratch? User will again have to deal with face removal. Can we be better than this? Can we, instead of removing face_id, remove some other identifier, let's say - (image_id, left, right, top, bottom)? Have a table removed_faces and keep those? But it will not help if image changes, we would still have to reset this too (as we don't know if image filename is the same and it is completely different picture)

@DerOetzi
Copy link
Author

DerOetzi commented Jun 6, 2020

Thanks to all of your feedback. I didn't thought about reaching the goal with first draft. So don't worry about my work. For me as an agile software architect it's a lot of fun to do some experiments to get closer to a possible solution by discussing the results of the experiment. I always do things like try out and fail fast and try something else.

To me UI/UX is very important as well as clean code I think I have lot of experience to those especially UX and maybe can give some input.

First of all I'm a fan of the idea of managing table(s), but like I wrote in #158 in my opinion especially the removal of garbage faces should not only be taken into account for viewing reasons but although for clean up of "data garbage" for performance reasons. Keeping the database smaller, could for example help the clustering process and although performance of simple queries. So I'm still not 100 percent convinced whether leaving ML-output-data "as-is" is really the best approach. But as I said above I can think about a new experiment about this approach.

It is tempting to delete the face as you are doing, but keep in mind that a face can be an entire cluster. You can leave a person without faces, which breaks the integrity of the database. You must control this situation.

I already have come to this point, that there is the need of clean up persons table as well, but I was not sure. There is a TODO in my code in FaceMapper taking account of this question, which you answered now.

Integration UI Personal Site

For now don't worry about this. This view was developed to evaluate the quality of the clusters, but the main view should be the files and photos applications.

When we develop the people view in photos, this view will be removed. 😉
If the user wants to delete/invalidate/etc a face that opens the image and does it in the sidebar.

Removing this view in my opinion would be a step backwards in UX of the application. Let's take my collection of pictures. There are round about 20000 pictures at the moment with 28000 faces recognized, per shooting there about 100-200 new Pictures and because of lot of audience round about 100-300 new faces round about 80% unknown people. Approx 30 new correct clustered persons and 20 mixed clustered persons which are all unknown, 10-20 added to existing persons, and maybe some wrong added. Managing of the new 300 faces is much easier on a overview page like this view. For example removing a lot of them already by removing the 50 clusters is much faster, then clicking through 200 pictures and remove face by face. Although getting overview of persons and there faces is already very good at the moment, and could be improved with different sorting, filtering and some more list handling options. I would find it a shame to loose this entry point to recognized faces. In my opinion this entry point is as important as more possibilities at single picture editor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants