Skip to content
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

Add bincount and denseBincount op. #4348

Merged
merged 10 commits into from
Dec 8, 2020
Merged

Conversation

lina128
Copy link
Collaborator

@lina128 lina128 commented Dec 4, 2020

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

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.

Thank you for implementing those two ops!

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


tfjs-backend-cpu/src/kernels/DenseBincount_impl.ts, line 20 at r1 (raw file):

import {buffer, DataType, Rank, TensorBuffer, TypedArray, util} from '@tensorflow/tfjs-core';

export function bincountImpl(

should this file be named as bincount_impl.ts?


tfjs-converter/python/tensorflowjs/op_list/control.json, line 3 at r1 (raw file):

[
  {
    "tfOpName": "EmptyTensorList",

Is this from the other PR?


tfjs-converter/src/metadata_test.ts, line 32 at r1 (raw file):

    const knownUnmappedKernels = [
      'Const',
      'EmptyTensorList',

from the other PR?


tfjs-converter/src/operations/executors/control_executor.ts, line 273 at r1 (raw file):

    }
    case 'TensorListReserve':
    case 'EmptyTensorList': {

same here


tfjs-converter/src/operations/executors/control_executor_test.ts, line 693 at r1 (raw file):

  });

  describe('EmptyTensorList', () => {

same here


tfjs-converter/src/operations/op_list/control.ts, line 21 at r1 (raw file):

export const json: OpMapper[] = [
  {
    'tfOpName': 'EmptyTensorList',

same here


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

  util.assert(
      $x.dtype === 'int32',
      () => `Error in denseBincount: input ` +

bincount?


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

  util.assert(size >= 0, () => `size must be non-negative, but got ${size}.`);

  const inputs: BincountInputs = {x: $x, weights: $weights};

should you check the size matches weight dimension (except when the weight is zero sized)?


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

      () => `Error in denseBincount: input must be at most rank 2, but got ` +
          `rank ${$x.rank}.`);
  util.assert(size >= 0, () => `size must be non-negative, but got ${size}.`);

similar to bin count, should weight shape matches the size?


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

 *     greater than 1, then all values of `strides` must be 1.
 *
 * @doc {heading: 'Operations', subheading: 'Convolution'}

from another PR?

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.

Reviewed 32 of 32 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

@lina128 lina128 force-pushed the bincount branch 2 times, most recently from 65c0a31 to df183b3 Compare December 7, 2020 20:35
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: 0 of 1 approvals obtained (waiting on @pyu10055)


tfjs-converter/python/tensorflowjs/op_list/control.json, line 3 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

Is this from the other PR?

Merged other PRs, now these files have no change.


tfjs-converter/src/metadata_test.ts, line 32 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

from the other PR?

Merged other PRs, now this file has no change.


tfjs-converter/src/operations/executors/control_executor.ts, line 273 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

same here

Merged other PRs, now this file has no change.


tfjs-converter/src/operations/executors/control_executor_test.ts, line 693 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

same here

Merged other PRs, now this file has no change.


tfjs-converter/src/operations/op_list/control.ts, line 21 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

same here

Merged other PRs, now this file has no change.


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

Previously, pyu10055 (Ping Yu) wrote…

bincount?

Done.


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

Previously, pyu10055 (Ping Yu) wrote…

should you check the size matches weight dimension (except when the weight is zero sized)?

Done. Added tests too.


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

Previously, pyu10055 (Ping Yu) wrote…

similar to bin count, should weight shape matches the size?

Done.


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

Previously, pyu10055 (Ping Yu) wrote…

from another PR?

Just realized before subheading is wrong, fix it in this PR.


tfjs-backend-cpu/src/kernels/DenseBincount_impl.ts, line 20 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

should this file be named as bincount_impl.ts?

Done.

@lina128 lina128 requested a review from pyu10055 December 7, 2020 21:48
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.

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


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

Previously, lina128 (Na Li) wrote…

Done. Added tests too.

the size of the weight should be same as the size attribute not the input size?

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 @lina128 and @pyu10055)


tfjs-backend-cpu/src/kernels/Bincount_impl.ts, line 36 at r3 (raw file):

    }

    if (weightsSize > 0) {

Hi Ping, as discussed offline, I've changed the behavior to align with TF c++ implementation. For 1d, it will set weights to be zero for out of range reference. @pyu10055


tfjs-backend-cpu/src/kernels/Bincount_impl.ts, line 72 at r3 (raw file):

      } else {
        if (weightsBuf.size > 0) {
          // The behavior here is different than bincountImpl above. We don't

Added doccomment about the different behavior between TF c++ implementation and TFJS. @pyu10055

@lina128 lina128 merged commit ce10710 into tensorflow:master Dec 8, 2020
@lina128 lina128 deleted the bincount branch December 8, 2020 19:43
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.

2 participants