Skip to content

Shouldn't have a +1 in the NMS implementation for the boxes width/height computation ? #1872

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

Closed
viniciusarruda opened this issue Feb 11, 2020 · 6 comments

Comments

@viniciusarruda
Copy link

The standard is to have a bounding box defined as quoted here.

But in the NMS source code, there is no +1 when computing the areas and intersection values. This also leaves a bug in the case of getting union = 0, raising a NaN error when computing the iou.

If the code is correct, what am I missing ? Shouldn't the documentation explain this better ?

Thanks.

@bmanga
Copy link
Contributor

bmanga commented Feb 12, 2020

Same in the cuda implementation.

@fmassa
Copy link
Member

fmassa commented Feb 13, 2020

Hi,

Great questions!

We have answered those questions in #826 (comment) .

We handle NaN in the implementation by not clamping the area to zero, but if you do find an example where this fails please let us know!

Let us know if you have further questions!

@bmanga
Copy link
Contributor

bmanga commented Feb 13, 2020

@fmassa I think it would make sense to specify in the documentation (or somewhere) that (x1, y1) is inclusive and (x2, y2) is not. That would be simpler than trying to pin it as working with continuous coordinates IMO (that's what OpenCV also does).

@fmassa
Copy link
Member

fmassa commented Feb 14, 2020

@bmanga I think we can reference in the documentation the discussion thread in the attached link I sent, this way users have the full picture.

@bmanga
Copy link
Contributor

bmanga commented Feb 14, 2020

@fmassa Agreed, a quick explanation + link would be good. I can create a PR if you'd like.

@fmassa
Copy link
Member

fmassa commented Feb 14, 2020

Yes please!

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

No branches or pull requests

3 participants