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

fix boundary condition when calculating Haar features #4076

Merged
merged 4 commits into from
Aug 26, 2021

Conversation

WeiChungChang
Copy link
Contributor

@WeiChungChang WeiChungChang commented Aug 2, 2019

Description

Closes #4077. Closes #4818.

Fix boundary condition when calculating Haar features

Current # of Haar features is wrong. It can be verified by 2 ways

  1. check 2 rectangle horizontal.
    https://en.wikipedia.org/wiki/Haar-like_feature
feature_types = ['type-2-x']
# Extract all possible features
feature_coord, feature_type = \
   haar_like_feature_coord(width=2, height=1,
                           feature_type=feature_types)
print('feature_coord.sahpe ', feature_coord.shape)

Notice that we should have # = 1 for this case but get 0 now.

# Extract all possible features
feature_coord, feature_type = \
   haar_like_feature_coord(width=2, height=24,
                           feature_type=None)
print('feature_coord.sahpe ', feature_coord.shape)

Please refer to:
https://stackoverflow.com/questions/1707620/viola-jones-face-detection-claims-180k-features
The correct result should be 162,336.
However, current results is 161864,

The problem is we miss the largest height(or width) since for python,
for for dy in range(1, height) does NOT include height.

Result

Apply this fix, both test cases are correct.

Copy link
Member

@ahojnnes ahojnnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! This looks good to me. Is there any chance we can add a small unit test that tests for the correct number of features or similar?

@soupault
Copy link
Member

soupault commented Aug 6, 2019

Also, the current tests are failing badly. It would be good to start from them.

@emmanuelle
Copy link
Member

@glemaitre would you have the time to take a look at this PR?

@jni
Copy link
Member

jni commented Jan 14, 2020

perhaps @warmspringwinds could also weigh in here...

@warmspringwinds
Copy link
Contributor

warmspringwinds commented Jan 15, 2020

@jni Sure, no problem! -- let's see if @glemaitre is available first.

@glemaitre -- Sorry for bugging, If I recall correctly you worked on this feature?
Would you have time to look into this whenever you are free/available?
Thank you so much in advance!

@glemaitre
Copy link
Contributor

The tests will fail just because of the shape since the filter contains one pixel more. So they can easily be adapted:

E       (shapes (5,), (4,) mismatch)
E        x: array([-5, -4, -3, -2, -1])
E        y: array([-4., -3., -2., -1.])

Regarding the fix itself, I think that it makes sense. Basically, you would expect that filter.shape == (height, width) which is not the case right now.

@glemaitre
Copy link
Contributor

Do you want me to take over and modify the tests?

@warmspringwinds
Copy link
Contributor

warmspringwinds commented Jan 16, 2020

@glemaitre Thanks for helping out!

Just review the pull request so that it looks good to you
and yeah that would be great if we can also make the tests pass -- that would be awesome!
Thank you for your input again!

@WeiChungChang Thank you for your pull request!

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @WeiChungChang. The change to the range proposed looks correct to me. I double-checked that the numbers of features of each type now match those listed in Fig. 2 of the following IPOL paper:
https://www.ipol.im/pub/art/2014/104/

I updated the docstring and test output to reflect this change.

@grlee77 grlee77 added 🩹 type: Bug fix Fixes unexpected or incorrect behavior action: mrg+1 labels Aug 22, 2021
@grlee77 grlee77 added this to the 0.19 milestone Aug 22, 2021
@grlee77
Copy link
Contributor

grlee77 commented Aug 22, 2021

also closes #4818

Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @WeiChungChang for the fix and @grlee77 for updating the UT 😉

@rfezzani rfezzani merged commit 43d79b8 into scikit-image:main Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
9 participants