Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Dec 10, 2019

FEATURE Add support to NonMaxSuppressionV5: add arg softNmsSigma and implementation to nonMaxSuppression op.


This change is Reviewable

@lina128 lina128 requested review from nsthorat and pyu10055 December 10, 2019 00:50
@lina128 lina128 marked this pull request as ready for review December 10, 2019 00:50
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Great work on this Na! I don't have any major comments, this fits very well in line with our style :)

Just want to double check: you've made sure this matches TensorFlow python by testing in a colab or something of that sort?

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @lina128, @nsthorat, and @pyu10055)


tfjs-core/src/backends/array_util.ts, line 3 at r1 (raw file):

/**
 * @license
 * Copyright 2018 Google LLC. All Rights Reserved.

2019


tfjs-core/src/backends/array_util_test.ts, line 3 at r1 (raw file):

/**
 * @license
 * Copyright 2018 Google LLC. All Rights Reserved.

2019


tfjs-core/src/ops/image_ops.ts, line 148 at r1 (raw file):

/**
 * Performs non maximum suppression of bounding boxes based on
 * iou (intersection over union). This op also supports a Soft-NMS mode (c.f.

nit: put a break line before the new sentence

@lina128
Copy link
Collaborator Author

lina128 commented Dec 10, 2019

Thank you for the review Nikhil! I only made sure it matches with TensorFlow c++ code. I will test the python version and let you know. Good catch :)

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Great work Na! Are there any TF tests for NMSV5 we can include here?

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @lina128 and @pyu10055)


tfjs-core/src/ops/image_ops.ts, line 237 at r1 (raw file):

    scoreThreshold = Number.NEGATIVE_INFINITY;
  }
  if (softNmsSigma == null) {

make sure softNmsSigma is between 0 and 1?

@lina128
Copy link
Collaborator Author

lina128 commented Dec 10, 2019

Great work Na! Are there any TF tests for NMSV5 we can include here?

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

tfjs-core/src/ops/image_ops.ts, line 237 at r1 (raw file):

    scoreThreshold = Number.NEGATIVE_INFINITY;
  }
  if (softNmsSigma == null) {

make sure softNmsSigma is between 0 and 1?

Thank you for the review, Ping! Added the check.

@lina128 lina128 force-pushed the non_max_suppression branch from 9f17f4e to c23800e Compare December 10, 2019 19:09
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@lina128
Copy link
Collaborator Author

lina128 commented Dec 10, 2019

Great work on this Na! I don't have any major comments, this fits very well in line with our style :)

Just want to double check: you've made sure this matches TensorFlow python by testing in a colab or something of that sort?

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @lina128, @nsthorat, and @pyu10055)

tfjs-core/src/backends/array_util.ts, line 3 at r1 (raw file):

/**
 * @license
 * Copyright 2018 Google LLC. All Rights Reserved.

2019

tfjs-core/src/backends/array_util_test.ts, line 3 at r1 (raw file):

/**
 * @license
 * Copyright 2018 Google LLC. All Rights Reserved.

2019

tfjs-core/src/ops/image_ops.ts, line 148 at r1 (raw file):

/**
 * Performs non maximum suppression of bounding boxes based on
 * iou (intersection over union). This op also supports a Soft-NMS mode (c.f.

nit: put a break line before the new sentence

Done

@lina128
Copy link
Collaborator Author

lina128 commented Dec 10, 2019

@nsthorat and @pyu10055 , the python version outputs the same result as the added test in image_ops_test.ts > 'select from three clusters with SoftNMS', here is the Colab link for the Python result: https://colab.research.google.com/drive/1ivLrOjw70UccrM6ENszizE1mb1mGB7RF

@lina128 lina128 merged commit a5d9b00 into tensorflow:master Dec 10, 2019
@mattyaodoit
Copy link

Hi, I am using the latest version of tfjs and still getting this error from my browser
image

@lina128
Copy link
Collaborator Author

lina128 commented Jan 29, 2020

Hi, I am using the latest version of tfjs and still getting this error from my browser
image

Hi @mattyao1984, this PR implemented the cpu kernel and gpu kernel for NonMaxSuppressionV5 op, and this change is included in tfjs-core 1.5.1 and tfjs 1.5.1. But we haven't included the op in tfjs-converter yet. We will do another release tomorrow to include everything. Stay tuned.

@mattyaodoit
Copy link

@lina128 Thanks!

@lina128 lina128 deleted the non_max_suppression branch April 14, 2020 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants