Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Jul 14, 2020

Fixes #2450


This change is Reviewable

@lina128 lina128 force-pushed the nonmaxsuppression branch from 841901f to c5a2fa4 Compare July 14, 2020 17:09
@lina128 lina128 marked this pull request as ready for review July 15, 2020 00:43
@lina128 lina128 requested review from annxingyuan and pyu10055 July 15, 2020 00:43
@lina128 lina128 changed the title Resolve merge conflict. Add NonMaxSuppressionV4. Jul 15, 2020
Copy link
Contributor

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

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

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


tfjs-backend-wasm/src/kernels/NonMaxSuppression_util.ts, line 25 at r1 (raw file):

  selectedSize: number;
  pSelectedScores: number;
  pValidOutputs: number;

pNumValidOutputs?


tfjs-backend-wasm/src/kernels/NonMaxSuppressionV4.ts, line 31 at r1 (raw file):

function setup(backend: BackendWasm): void {
  wasmFunc = backend.wasm.cwrap(
      'NonMaxSuppressionV4',

use symbol here


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

      iouThreshold,
      scoreThreshold,
      0,                  /* softNmsSigma */

can we put the comments before the comma for consistency?


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

  const elemsToPad = maxOutputSize - validOutputs;

  if (padToMaxOutputSize && elemsToPad > 0) {

padToMaxOutputSize != null


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

  if (returnValidOutputs) {
    result['validOutputs'] = scalar(validOutputs, 'int32');

should this be numValidOutputs for consistency? also makes it clear that it's a scalar


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

  const $maxOutputSize = params.maxOutputSize;
  const $iouThreshold = params.iouThreshold;
  const $scoreThreshold = params.scoreThreshold;

would it be easier just to destructure params and pass maxOutputSize, iouThreshold, scoreThreshold directly into nmsv4impl?

Copy link
Collaborator Author

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

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


tfjs-backend-wasm/src/kernels/NonMaxSuppressionV4.ts, line 31 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

use symbol here

Done.


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

Previously, annxingyuan (Ann Yuan) wrote…

can we put the comments before the comma for consistency?

Done.


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

Previously, annxingyuan (Ann Yuan) wrote…

padToMaxOutputSize != null

It's a boolean value, and we need to check truthy.


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

Previously, annxingyuan (Ann Yuan) wrote…

should this be numValidOutputs for consistency? also makes it clear that it's a scalar

The api output name is validOutputs, so I change everything to that. Is there a naming convention for results that represent numbers should start with num_?


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

Previously, annxingyuan (Ann Yuan) wrote…

would it be easier just to destructure params and pass maxOutputSize, iouThreshold, scoreThreshold directly into nmsv4impl?

That will mutate the inputs. Prefer to keep the input immutable.

@lina128 lina128 force-pushed the nonmaxsuppression branch from f82ad7a to e30be8c Compare July 17, 2020 00:38
@lina128 lina128 merged commit f65f650 into tensorflow:master Jul 17, 2020
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.

Unsupported Ops: NonMaxSuppresionV4 when converting model

3 participants