Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Add unstack op #1018

Merged
merged 3 commits into from
May 16, 2018
Merged

Add unstack op #1018

merged 3 commits into from
May 16, 2018

Conversation

Lewuathe
Copy link
Contributor

@Lewuathe Lewuathe commented May 4, 2018

Purpose

Some model including MobileNet requires unstack op (tensorflow/tfjs#188). Since unpack is depracated and renamed to unstack, TensorFlow.js should also use the same name convention.

see: tf.unstack


This change is Reviewable

@nsthorat
Copy link
Contributor

nsthorat commented May 4, 2018

Thanks!


Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.


src/ops/array_ops.ts, line 1031 at r1 (raw file):

  }

  static unstack<T extends Tensor>(

Can you add a jsdoc here with a code snippet? Put this along side stack in docs

Also add @operation so we clean up intermediates.


src/ops/array_ops_test.ts, line 2113 at r1 (raw file):

  });
});

can you add one unit tests with num = 3, ideally with


src/ops/array_ops_test.ts, line 2129 at r1 (raw file):

  it('unstack by axis=1', () => {
    const x = tf.tensor2d([1, 2, 3, 4, 5, 6, 7, 8], [2, 4]);
    const res = tf.unstack(x, 4, 1);

can you assert the other values? res[1], res[2] etc and their rank and shape? Here and elsewhere


Comments from Reviewable

@dsmilkov dsmilkov self-requested a review May 13, 2018 02:48
@dsmilkov
Copy link
Contributor

:lgtm_strong: Nice work. Sorry for the delay. Nikhil and I were traveling last week.

I noticed you addressed Nikhil's comments. I also left couple comments, one about removing the num parameter since in TF.js the shape is always known.

Not actionable in this PR: Having unstack implemented using slice is fine for now but we should consider adding this as a backend method and implement it separately for webgl and cpu, and nodejs, especially since Tensorflow has it as a separate kernel (unpack which we can call in our nodejs backend directly.


Reviewed 2 of 4 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/ops/array_ops.ts, line 1040 at r2 (raw file):

   *
   * @param value A tensor object.
   * @param num The number of unstacked tensors.

can you remove the num parameter since in TF.js the shape is always known (the engine is eager mode)?


src/ops/array_ops.ts, line 1056 at r2 (raw file):

    const outputShape: number[]
        = Array(value.rank - 1).fill(0);

the 2 lines above can fit in a single line


Comments from Reviewable

@dsmilkov
Copy link
Contributor

:lgtm_strong: Thank you Kai!


Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


Comments from Reviewable

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants