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

Support tensors rank 5 #1022

Merged
merged 9 commits into from
Jun 2, 2018
Merged

Support tensors rank 5 #1022

merged 9 commits into from
Jun 2, 2018

Conversation

zaidalyafeai
Copy link
Contributor

@zaidalyafeai zaidalyafeai commented May 6, 2018

Added tensors rank 5. This is still a work in progress. The shader_compiler.ts file needs to be further revised ... Many gpu operations needs to be changed like reverse, transpose, slice ...


This change is Reviewable

@zaidalyafeai zaidalyafeai changed the title Added tensors rank 5 WIP Added tensors rank 5 May 6, 2018
@zaidalyafeai zaidalyafeai changed the title WIP Added tensors rank 5 Added tensors rank 5 May 8, 2018
@zaidalyafeai zaidalyafeai changed the title Added tensors rank 5 Support tensors rank 5 May 8, 2018
@zaidalyafeai
Copy link
Contributor Author

Does anybody have time to review this? I am traveling in a few days and will not have time to work on the comments.

@dsmilkov
Copy link
Contributor

@zaidalyafeai , this is a great start to adding 5D. Thank you for this change. Since you already modified pad, reverse, transpose and tile to work with 5D in this PR, you'll have to add tests for those ops as well. I only see 5d tests for tile. Let me know once you add those tests and then this will be ready to go.

I'm happy to also review future PRs that change other ops to work with 5D. Thank you!


Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


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

   * ```js
   * // Pass a flat array and specify a shape.
   * tf.tensor4d([1, 2, 3, 4, 5, 6], [1, 2, 2, 2, 1]).print();

call tensor5d


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

      dtype: DataType = 'float32'): Tensor5D {
    if (shape != null && shape.length !== 5) {
      throw new Error('tensor5d() requires shape to have four numbers');

fix error message to say five numbers instead of four


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

      constantValue = 0): Tensor5D {
    util.assert(
        paddings.length === 4 && paddings[0].length === 2 &&

fix these checks to be for 5d (the current values are for the 4d case)


Comments from Reviewable

@zaidalyafeai
Copy link
Contributor Author

@dsmilkov, I will not have the time to add unit tests for reverse, pad and transpose because I am getting ready to travel. Should I revert the changes I made to those operations ( they are minimal) ?

@dsmilkov
Copy link
Contributor

No worries. Reverting those sounds good and then we can continue later with the follow up changes. Safe travels!

@nsthorat
Copy link
Contributor

Let me know when this is ready to look at again.

@zaidalyafeai
Copy link
Contributor Author

@nsthorat I already commited my final changes.

@nsthorat
Copy link
Contributor

It's looking good! Would be good to add some unit tests for operations with 5D Tensors that aren't just construction, specifically for operations that operate on any rank (mean, unary ops like square, etc).


Review status: 5 of 8 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from Reviewable

@zaidalyafeai
Copy link
Contributor Author

@nsthorat, It is not clear to me what operations should I focus on. The unary ops file only contains unit tests for tensors of rank less than 3.

@rodrigob
Copy link

it is not clear to me what operations should I focus on.
why not start with mean and square and see from there ?

@nsthorat nsthorat requested a review from dsmilkov May 23, 2018 23:39
@zaidalyafeai
Copy link
Contributor Author

Since transpose is necessary for tensorflow/tfjs#161 I implemented it . Maybe we can merge this and support the other gpu operations once needed ?

@zaidalyafeai
Copy link
Contributor Author

@nsthorat @dsmilkov I added unit tests for abs and relu to make sure 5D tensors work fine. Let me know if we need more...

@dsmilkov
Copy link
Contributor

dsmilkov commented Jun 2, 2018

:lgtm_strong: Thank you!! And sorry for the wait


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@tensorflow tensorflow deleted a comment from googlebot Jun 2, 2018
@dsmilkov
Copy link
Contributor

dsmilkov commented Jun 2, 2018

One unrelated test fails - it's a known test tracked in tensorflow/tfjs#377 which shouldn't block this PR.

@dsmilkov dsmilkov merged commit d82562d into tensorflow:master Jun 2, 2018
@zaidalyafeai
Copy link
Contributor Author

Awesome 😀 glad it worked.

@zaidalyafeai zaidalyafeai deleted the zaidalyafeai branch June 4, 2018 11:44
@zaidalyafeai
Copy link
Contributor Author

@dsmilkov, time to work on the remaining gpu operations ?

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.

4 participants