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

Add math.concat[1-4]D and math.slice[1-4]D #151

Merged
merged 7 commits into from
Sep 27, 2017
Merged

Add math.concat[1-4]D and math.slice[1-4]D #151

merged 7 commits into from
Sep 27, 2017

Conversation

dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Sep 25, 2017

Also update math.basicLSTMCell to use slice1D instead of slice3D resulting in more readable code and more efficient shaders.

This change is Reviewable

@nsthorat
Copy link
Contributor

:lgtm_strong:

This is such a nice PR. Few minor comments, but stamped!


Reviewed 25 of 25 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


src/math/concat_util.ts, line 21 at r1 (raw file):

export function assertParams(aShape: number[], bShape: number[], axis: number) {
  const rank = aShape.length;

call this aRank


src/math/math_cpu_test.ts, line 129 at r1 (raw file):

    expect(result.getValues()).toEqual(new Float32Array([4, 8]));
  });
});

4d test?


src/math/math_gpu_test.ts, line 298 at r1 (raw file):

    expect(result.getValues()).toEqual(new Float32Array([4, 8]));
  });
});

4d test


src/math/slice_util.ts, line 22 at r1 (raw file):

import {NDArray} from './ndarray';

export function assertParams(

how about assertParamsValid?


src/math/webgl/concat_gpu_test.ts, line 293 at r1 (raw file):

  return result;
}

4d test?


src/math/webgl/slice_gpu_test.ts, line 209 at r1 (raw file):

    return result;
  }
});

4d test


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

Review status: 17 of 26 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.


src/math/concat_util.ts, line 21 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

call this aRank

Done.


src/math/math_cpu_test.ts, line 129 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

4d test?

Done.


src/math/math_gpu_test.ts, line 298 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

4d test

Done.


src/math/slice_util.ts, line 22 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

how about assertParamsValid?

Done.


src/math/webgl/concat_gpu_test.ts, line 293 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

4d test?

Done.


src/math/webgl/slice_gpu_test.ts, line 209 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

4d test

Done.


Comments from Reviewable

@dsmilkov dsmilkov merged commit 5fadaaa into master Sep 27, 2017
@dsmilkov dsmilkov deleted the rnn branch September 27, 2017 21:10
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
* add slice1d

* fix lstm bug

* generalize concat to support 1-4D and add slice[1-4]D

* update .gitignore

* add slice implementations for math_cpu and tests

* address comments

* Merge master into rnn
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.

2 participants