broadinstitute / keras-rcnn

Keras package for region-based convolutional neural networks (RCNNs)
Other
555 stars 222 forks source link

Fix IoU definition #48

Closed yanfengliu closed 7 years ago

yanfengliu commented 7 years ago

This PR fixes the definition of IoU discussed in this issue.

yanfengliu commented 7 years ago

It gave the following error, but I did not touch that part.

import keras_resnet.block.temporal
ImportError: No module named block.temporal
hgaiser commented 7 years ago

For the error, see https://github.com/broadinstitute/keras-rcnn/pull/46

yhenon commented 7 years ago

This calls intersection() twice with the same arguments, which isn't that desirable. IMO a better way is:

def union(au, bu, area_intersection):
    area_a = (au[2] - au[0]) * (au[3] - au[1])
    area_b = (bu[2] - bu[0]) * (bu[3] - bu[1])
    area_union = area_a + area_b - area_intersection
    return area_union

def intersection(ai, bi):
    x = max(ai[0], bi[0])
    y = max(ai[1], bi[1])
    w = min(ai[2], bi[2]) - x
    h = min(ai[3], bi[3]) - y
    return max(0, w*h)

def iou(a, b):
    # a and b should be (x1,y1,x2,y2)

    if a[0] >= a[2] or a[1] >= a[3] or b[0] >= b[2] or b[1] >= b[3]:
        return 0.0

    area_i = intersection(a, b)
    area_u = union(a, b, area_i)

    return float(area_i) / float(area_u)
yanfengliu commented 7 years ago

@hgaiser Thanks! I merged your pull request into mine just now

yanfengliu commented 7 years ago

@yhenon Thank you for the suggestion! I will change the code a little bit.

hgaiser commented 7 years ago

Actually I think it'd be better if this PR only contains the fixes for IOU, not also the fixes for keras-resnet.

On Jul 24, 2017 21:45, "Yanfeng Liu" notifications@github.com wrote:

@hgaiser https://github.com/hgaiser Thanks! I merged your pull request into mine just now

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/broadinstitute/keras-rcnn/pull/48#issuecomment-317533504, or mute the thread https://github.com/notifications/unsubscribe-auth/AArtap1JsibplYxDkDc6PUw6XhWzjEdvks5sRPRkgaJpZM4OhmL1 .

codecov-io commented 7 years ago

Codecov Report

Merging #48 into master will increase coverage by 0.19%. The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage   53.64%   53.83%   +0.19%     
==========================================
  Files          17       17              
  Lines         563      561       -2     
==========================================
  Hits          302      302              
+ Misses        261      259       -2
Impacted Files Coverage Δ
keras_rcnn/preprocessing/_object_detection.py 0% <0%> (ø) :arrow_up:
keras_rcnn/classifiers/resnet.py 94.44% <100%> (ø) :arrow_up:
keras_rcnn/models.py 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e341198...f6336b6. Read the comment docs.

yanfengliu commented 7 years ago

@hgaiser Sounds good. I will revert the commit.

hgaiser commented 7 years ago

Nice :) if you can squash the commits to one clean commit, you got my thumbs up.

yanfengliu commented 7 years ago

@hgaiser I tried to squash the commits but they have been pushed, so I don't know if there is some other way to change how it looks online. Sorry for the messy commits. I can close this one and open a new one if necessary.