-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Feat/batch predict age and gender #1396
base: master
Are you sure you want to change the base?
Conversation
Bump |
I don't support having another predicts function. Instead, you can add that logic under predict. 1- predict accepts both single image and list of images as img: Union[np.ndarray, List[np.ndarray]] 2- in predict function, you can check the type of img, and redirect it to your logic if it is list as if isinstance(img, np.ndarray):
# put old predict logic here
elif isinstance(img, np.ndarray):
# put your batch processing logic here 3- this new logic is worth to have its own unit tests. possibly, you can add some unit tests here. 4- return type of predict should be Union[np.float64, np.ndarray] 5- You should also update the interface in DeepFace.py |
…or Age and Gender models
Feat/add multi face test
[update] make Race and Emotion batch prediction
[update] merge predicting funcs
deepface/models/demography/Age.py
Outdated
""" | ||
# Convert to numpy array if input is list | ||
if isinstance(img, list): | ||
imgs = np.array(img) |
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.
cast img to img itself please, in that way we will not need else condition
deepface/models/demography/Age.py
Outdated
imgs = img | ||
|
||
# Remove batch dimension if exists | ||
imgs = imgs.squeeze() |
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.
you squeezed imgs and then expand its dimension, instead can we decide the input is single or many images when predict called?
if isinstance(img, list):
# then it is many images
elif len(img.shape) == 3:
# then it is single image
elif len(img.shape) == 4 and img.shape[0] == 1:
# then it is single image
elif len(img.shape) == 4 and img.shape[0] > 1:
# then it is many images
else:
raise error
Actions failed because of linting - link
|
imgs = np.expand_dims(imgs, axis=0) | ||
|
||
# Batch prediction | ||
age_predictions = self.model.predict_on_batch(imgs) |
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.
model.predict
causes memory issue when it is called in a for loop, that is why we call it as self.model(img, training=False).numpy()[0, :]
in your design, if this is called in a for loop, still it will cause memory problem.
IMO, if it is single image, we should call self.model(img, training=False).numpy()[0, :]
, it is many faces then call self.model.predict_on_batch
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.
Thank you for sharing your perspective on this matter.
We found the issue you mentioned is also mentioned in this page: tensorflow/tensorflow#44711. We believe it’s being resolved.
Furthermore, if we can utilize the batch prediction method provided in this PR, we may be able to avoid repeatedly calling the predict
function within a loop of unrolled batch images, which is the root cause of the memory issue you described.
We recommend that you consider retaining our batch prediction 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.
hey, even though this is sorted in newer tf versions, many users using old tf versions raise tickets about this problem. so, we should consider the people using older tf version. that is why, i recommend to use self.model(img, training=False).numpy()[0, :]
for single images, and self.model.predict_on_batch
for batches.
deepface/modules/demography.py
Outdated
sum_of_predictions = race_predictions.sum() | ||
# Convert the list of valid faces to a numpy array | ||
faces_array = np.array(valid_faces) | ||
resp_objects = [{} for _ in range(len(valid_faces))] |
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 do not create resp_objects in advance, instead create resp_object in the loop and append it to resp_objects similar to previous design
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.
i really don't like to set resp_objects with idx, recommend something in the code
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.
i simplified it with zip
and update
in 52a38ba
it looks more clean and pythonic now :)
As the following code review comment suggested: serengil#1396 (comment)
Fix: Image batch dimension not expanded.
|
||
obj["dominant_race"] = Race.labels[np.argmax(race_predictions)] | ||
# Create placeholder response objects for each face | ||
for _ in range(len(valid_faces)): |
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 set resp_objects to an empty list here as
resp_objects = []
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.
resolved in #1396 (comment)
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.
seems this is not resovled.
before for loop, you should create a resp_objects as resp_objects = []
in for loop, you should create resp_object as resp_object = {}
and set its features.
in summary, instead resp_objects[idx]["emotion"] = {}
, we should have something like resp_object["emotion"] = {}
age_predictions = model.predict(faces_array) | ||
|
||
for idx, age in enumerate(age_predictions): | ||
resp_objects[idx]["age"] = int(age) |
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.
set resp_object instead of resp_objects as
resp_object["age"] = int(age)
do this for each attribute please
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 referring to whether the attribute mentioned refers to the percentage of emotion
or gender
?
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.
i don't want you to use idx at all.
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.
i initial the resp_objects
renamed as results
with face region and confidence.
https://github.com/NatLee/deepface/blob/52a38ba21a9b7edb331e2a6f25e68115ca1a663c/deepface/modules/demography.py#L155-L157
and use update
function to write result of models like this
https://github.com/NatLee/deepface/blob/52a38ba21a9b7edb331e2a6f25e68115ca1a663c/deepface/modules/demography.py#L180-L193
the newest version didn't use any idx
in the code.
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.
ahh the newest version is in the other branch which hasnt been merged yet
Four-dimensional numpy array (n, 224, 224, 3) | ||
""" | ||
if isinstance(img, list): # Convert from list to image batch. | ||
image_batch = np.array(img) |
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.
if you use it like this, you don't have to have if and else blocks.
img = alpha_embedding = np.asarray(img)
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.
i think @h-alice 's concern is likely about the img
variable being compromised. 🤔
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.
@serengil We appreciate your advice.
We will implement your recommended approach later.
Tickets
#441
#678
#1069
#1101
What has been done
With this PR, new predicts function to support batch predictions.
How to test
Use this class, user can load model and predict in batch.
Time Costs for 10 Images:
Here's my test script:
predicts
is placed in age and gender clients to keep theDeepFace.analyze
function logic.Thank you for taking time to go through this feedback. :)