-
Notifications
You must be signed in to change notification settings - Fork 34
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
add-grayscale-equalized #62
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.
The PR diff size of 33366 lines exceeds the maximum allowed for the inline comments feature.
0c1ea58
to
433d6ed
Compare
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.
The PR diff size of 33366 lines exceeds the maximum allowed for the inline comments feature.
433d6ed
to
41fbc77
Compare
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.
The PR diff size of 33364 lines exceeds the maximum allowed for the inline comments feature.
41fbc77
to
534ea4b
Compare
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.
The PR diff size of 33366 lines exceeds the maximum allowed for the inline comments feature.
534ea4b
to
3572db5
Compare
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.
The PR diff size of 33362 lines exceeds the maximum allowed for the inline comments feature.
3572db5
to
ae217fa
Compare
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.
The PR diff size of 33369 lines exceeds the maximum allowed for the inline comments feature.
ae217fa
to
fb2d2ba
Compare
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.
The PR diff size of 33369 lines exceeds the maximum allowed for the inline comments feature.
Pull Request Test Coverage Report for Build 3620736147
💛 - Coveralls |
fb2d2ba
to
7046aa7
Compare
7046aa7
to
3bd2aa3
Compare
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.
The PR diff size of 33430 lines exceeds the maximum allowed for the inline comments feature.
3bd2aa3
to
8c66fb1
Compare
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.
The PR diff size of 33428 lines exceeds the maximum allowed for the inline comments feature.
LGTM 🎉 |
8c66fb1
to
fc4284b
Compare
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.
The PR diff size of 33426 lines exceeds the maximum allowed for the inline comments feature.
fc4284b
to
009f472
Compare
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.
The PR diff size of 33427 lines exceeds the maximum allowed for the inline comments feature.
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.
LGTM 🚀
@fbtravi, we can add your new detector as part of |
hi @guilhermef Yes, I could put it in all, but the doubt would be here.
With two face decteres which points do we use? Do we use the points of both? |
@fbtravi yes, it's not a problem to have overlapping detection unless one of them is giving false positives. |
c2252ae
to
465273c
Compare
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.
The PR diff size of 33470 lines exceeds the maximum allowed for the inline comments feature.
465273c
to
25bcbc8
Compare
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.
The PR diff size of 33477 lines exceeds the maximum allowed for the inline comments feature.
Hi @guilhermef, changes made. |
Awesome work @fbtravi |
remotecv/utils.py
Outdated
# -*- coding: utf-8 -*- | ||
|
||
# thumbor imaging service | ||
# https://github.com/globocom/thumbor/wiki | ||
|
||
# Licensed under the MIT license: | ||
# http://www.opensource.org/licenses/mit-license | ||
# Copyright (c) 2011 globo.com timehome@corp.globo.com | ||
# Copyright (c) 2022 globo.com <thumbor@g.globo> |
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 should not change year from copyright on old files.
# -*- coding: utf-8 -*- | ||
|
||
# thumbor imaging service | ||
# https://github.com/globocom/thumbor/wiki | ||
|
||
# Licensed under the MIT license: | ||
# http://www.opensource.org/licenses/mit-license | ||
# Copyright (c) 2011 globo.com timehome@corp.globo.com | ||
# Copyright (c) 2022 globo.com <thumbor@g.globo> |
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 should not change year from copyright on old files.
# -*- coding: utf-8 -*- | ||
|
||
# thumbor imaging service | ||
# https://github.com/globocom/thumbor/wiki | ||
|
||
# Licensed under the MIT license: | ||
# http://www.opensource.org/licenses/mit-license | ||
# Copyright (c) 2011 globo.com timehome@corp.globo.com | ||
# Copyright (c) 2022 globo.com <thumbor@g.globo> |
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 should not change year from copyright on old files.
Sorry to come late to the party, but I don't think we should merge this. The goal of the entire team of committers should be to reduce thumbor's codebase, not increase it. This detector seems like an awesome addition to the ecosystem, we can even host it in thumbor's org as a separate project, but I'd love if we could trim thumbor's codebase. The more code we have the slower we move. Sorry for taking so long to review this. |
@heynemann, given the data presented on #59, shouldn't we make this the default behavior for face detection instead of adding a new detector? |
If that’s the case we can improve our current face detector.
On Sun, 4 Dec 2022 at 06:55 Guilherme Souza ***@***.***> wrote:
@heynemann <https://github.com/heynemann>, given the data presented on #59
<#59>, shouldn't we make this
the default behavior for face detection instead of adding a new detector?
It seems that this would improve the accuracy.
—
Reply to this email directly, view it on GitHub
<#62 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAO4JNWLKB6UN6QBRGLKNLWLRS7XANCNFSM6AAAAAASHAFGEI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Bernardo Heynemann
|
The first idea was to create a new detector and sharing almost all the code, not changing the existing one so as not to break any users. If possible, let me know so I can continue with the implementation or not. |
@fbtravi let's refactor the existing face detection to include your change, then we can release it as a major change. |
25bcbc8
to
2e23ddc
Compare
Code Climate has analyzed commit 2e23ddc and detected 0 issues on this pull request. View more on Code Climate. |
Would they be able to analyze the changes? We had a decrease in the lines tested, but this compared to my last commit, the last merge in master is like this |
Great job @fbtravi 🚀 |
Added new face detector, the 'face_grayscale_equalized_detector'.
This detector can be used as the 'face_detector', but it is not part of the 'all', this happens so as not to change the use of whom I have the 'all' instantiated.
Issue