Verification #1309
Verification #1309
Conversation
deepface/modules/verification.py
Outdated
| ) | ||
| except ValueError as err: | ||
| raise ValueError("Exception while processing img2_path") from err | ||
| def extract_faces_from_img(img_path, index=1): |
There was a problem hiding this comment.
1- this function finds embeddings and facial areas. we should give the function a proper name.
2- please add a docstring to this function
3- types of this function are missing. img_path: Union[str, np.ndarray] and index: int. Besides, should we really set a default value to index?
4- return type of this function is missing. it should return Tuple[List[List[float]], List[dict]]
There was a problem hiding this comment.
I thought that since the function is internal and auxiliary, it wasn't necessary to format it according to the function formatting rules. I'll fix it without any problem.
There was a problem hiding this comment.
Of course, it is not necessary. Still, I strongly recommend to have type hinting.
deepface/modules/verification.py
Outdated
| distance = float(distances[min_index]) # best distance | ||
| facial_areas = facial_areas[min_index] | ||
| distance = float(min_distance) | ||
| facial_areas = (no_facial_area, no_facial_area) if None in (min_idx, min_idy) else \ |
There was a problem hiding this comment.
what if img1_facial_areas[min_idx] is None and img2_facial_areas[min_idy] is not None?
In previous design, we were setting no_facial_area's values to 1st item, and img2_facial_areas[min_idy]'s value to 2nd item.
In your design, no_facial_area's values are not set, instead you are setting None. Am I right?
There was a problem hiding this comment.
I think the following code snippet will resolve that issue
facial_areas = (
no_facial_area if min_idx is None else img1_facial_areas[min_idx] or no_facial_area,
no_facial_area if min_idy is None else img2_facial_areas[min_idy] or no_facial_area,
)There was a problem hiding this comment.
As I understand it, img1_facial_areas[min_idx] and img2_facial_areas[min_idy] will never be None, because if a face is not found, the area of the entire image is still returned. Therefore, the code from the previous design, img1_facial_areas[idx] or no_facial_area, will never return no_facial_area.
That's why I thought that if we don't find faces in at least one of the photos, we shouldn't display anything at all.
The code you suggested really solves the problem, thank you!
There was a problem hiding this comment.
Maybe we don't need or no_facial_area in the end of statements? Because no_facial_area if min_idx is None else img1_facial_areas[min_idx] will return Truthy anyway.
facial_areas = (
no_facial_area if min_idx is None else img1_facial_areas[min_idx],
no_facial_area if min_idy is None else img2_facial_areas[min_idy],
)There was a problem hiding this comment.
Actually, img1_facial_areas[idx] can be None from this line
https://github.com/serengil/deepface/blob/master/deepface/modules/verification.py#L166
There was a problem hiding this comment.
No, the problem occurs when min_idx is 0 but img1_facial_areas[0] is None. We still need to return the dictionary, but the code snippet you recommended returns None.
This happens when you feed an embedding to img_path
There was a problem hiding this comment.
Yes, you are right. I didn't mention, that img1_facial_areas[idx] can return None. Sorry, my bad.
What do you think, maybe, it is better to do something like this?
img_facial_areas = [no_facial_area]instead of
img_facial_areas = [None]There was a problem hiding this comment.
yeah, this will also work
i hope this is the only point facial area can be none - https://github.com/serengil/deepface/blob/master/deepface/modules/verification.py#L166
|
Very good spot btw, I don't like that repeating code and have been planning to fix it. You acted faster than me. Cheers. |
|
Linting failed |
|
Would you please add an unit test for facial areas checking to https://github.com/serengil/deepface/blob/master/tests/test_verify.py#L123 ? assert result["facial_areas"]["img1"] is not None
assert result["facial_areas"]["img2"] is not None
assert isinstance(result["facial_areas"]["img1"], dict)
assert isinstance(result["facial_areas"]["img2"], dict)
assert "x" in result["facial_areas"]["img1"].keys()
assert "y" in result["facial_areas"]["img1"].keys()
assert "w" in result["facial_areas"]["img1"].keys()
assert "h" in result["facial_areas"]["img1"].keys()
assert "left_eye" in result["facial_areas"]["img1"].keys()
assert "right_eye" in result["facial_areas"]["img1"].keys()
assert "x" in result["facial_areas"]["img2"].keys()
assert "y" in result["facial_areas"]["img2"].keys()
assert "w" in result["facial_areas"]["img2"].keys()
assert "h" in result["facial_areas"]["img2"].keys()
assert "left_eye" in result["facial_areas"]["img2"].keys()
assert "right_eye" in result["facial_areas"]["img2"].keys() |
|
|
||
| if silent is False: | ||
| logger.warn( | ||
| "You passed 1st image as pre-calculated embeddings." |
There was a problem hiding this comment.
1st image term should not be static. Would you please get it from index arg?
| if len(img_path) != dims: | ||
| raise ValueError( | ||
| f"embeddings of {model_name} should have {dims} dimensions," | ||
| f" but it has {len(img_path)} dimensions input" |
There was a problem hiding this comment.
shall we mention the index here as well?
|
LGTM, I added a couple of minor comments but I can add them later. Cheers! |
What has been done
With this PR:
How to test