Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Dec 12, 2019

  • Add the op using modular pattern
  • Create new folder for all new kernels

Note: Tested with latest core at e5a0c84


This change is Reviewable

@lina128 lina128 requested a review from kangyizhang December 12, 2019 18:12
@lina128 lina128 requested review from dsmilkov and nsthorat December 19, 2019 22:00
@lina128 lina128 marked this pull request as ready for review December 19, 2019 22:19
@lina128 lina128 requested a review from pyu10055 December 19, 2019 22:19
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.

Awesome! Can you update the dependency to 1.5.0 to have the tests run?

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @kangyizhang, @lina128, @nsthorat, and @pyu10055)


tfjs-node/src/kernels/non_max_suppression_v5.ts, line 41 at r1 (raw file):

    const {maxOutputSize, iouThreshold, scoreThreshold, softNmsSigma} =
        attrs as NonMaxSuppressionWithScoreAttrs;
    const maxOutputSizeTensor = scalar(maxOutputSize, 'int32');

to do this modularization properly we shouldn't have a dependency on tensors here, we should just operate entirely dataid's. This probably means some refactoring of executeMultipleOps

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

LGTM!

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


tfjs-node/src/kernels/non_max_suppression_v5.ts, line 41 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

to do this modularization properly we shouldn't have a dependency on tensors here, we should just operate entirely dataid's. This probably means some refactoring of executeMultipleOps

You or I can do that in a separate PR, since the node backend needs refactor (we never refactored it to disentangle tensors from tensor handles)

@lina128 lina128 merged commit 0bddd3c into tensorflow:master Dec 20, 2019
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.

4 participants