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 negative values in HOG calculation #606

Merged
merged 2 commits into from Feb 10, 2016

Conversation

Projects
None yet
2 participants
@patricksnape
Contributor

patricksnape commented Jul 6, 2015

Values were not being rounded correctly due to truncation
of the interger cell size in pixels. This corrects that
by using the original code used by @nontas in Matlab.

Fix negative values in HOG calculation
Values were not being rounded correctly due to truncation
of the interger cell size in pixels. This corrects that
by using the original code used by @nontas in Matlab.
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jul 7, 2015

This segfaults... Needs more investigaton - do not merge.

Trying to fix the negative values and avoid segfault
The Matlab code crashes as well apparently - it is not good
at generating correct values. This maintains our previous
indxing logic, but changes the interpolation code to use
fractional distance from the center of the bins. This
stops negative values. However, we need to check the bins
are getting interpolated correctly due to the unusual way
that they were being interpolated previously.
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jul 27, 2015

No longer segfaults - but now I question it's validity. I think we should look at totally replacing the HOG code :(

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Feb 1, 2016

In general, I have not investigated the correctness of this branch - however - there are so many implementations of HOG anyway that I don't think this really hurts. From what I've seen this doesn't affect fitting performance at all and it stops a potential segfault. Thoughts on just merging this and then coming back to re-implementing HOG/LBP in the future @jabooth @nontas?

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Feb 10, 2016

@jalabort said he used this branch and thought the results seemed OK. I'm leaning towards merging this in for the next release to stop the wrong behaviour?

@jalabort

This comment has been minimized.

Member

jalabort commented Feb 10, 2016

Yeah, I've been using this for a while and haven't had issues with it

patricksnape added a commit that referenced this pull request Feb 10, 2016

Merge pull request #606 from patricksnape/hog_neg_fix
Fix negative values in HOG calculation

@patricksnape patricksnape merged commit c033f43 into menpo:master Feb 10, 2016

4 checks passed

OS X MenpoBot Jenkins build passed No test results found.
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@patricksnape patricksnape deleted the patricksnape:hog_neg_fix branch Feb 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment