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

Add math.multinomial and math.oneHot #160

Merged
merged 4 commits into from
Sep 30, 2017
Merged

Add math.multinomial and math.oneHot #160

merged 4 commits into from
Sep 30, 2017

Conversation

dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Sep 29, 2017

This change is Reviewable

@dsmilkov dsmilkov changed the title Add math.multinomial which samples from a multinomial distribution Add math.multinomial and math.oneHot Sep 29, 2017
@nsthorat
Copy link
Contributor

:lgtm_strong:

This is a great CL! Nice work with the math CPU / GPU testing, this is so much better.

All minor comments below, but I stamped it!


Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


package.json, line 16 at r1 (raw file):

    "@types/jasmine": "~2.5.53",
    "@types/polymer": "~1.1.31",
    "@types/seedrandom": "^2.4.27",

use a tilde for consistency?


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

   *     match the location.
   */
  oneHot(indices: Array1D, depth: number, onValue?: number, offValue?: number):

why not just put the default here? onValue = 1, offValue = 0


src/math/math_cpu.ts, line 1022 at r1 (raw file):

      indices: Array1D, depth: number, onValue: number,
      offValue: number): Array2D {
    const res = new Float32Array(indices.size * depth);

you need to fill this with offValue


src/math/math_cpu.ts, line 1025 at r1 (raw file):

    for (let event = 0; event < indices.size; ++event) {
      res[event * depth + indices.get(event)] = 1;

s/1/onValue


src/math/multinomial_test.ts, line 33 at r1 (raw file):

  afterEach(() => {
    math.endScope(null);
  });

Here, and in the other file, can you add an afterAll and dispose the math so we don't create too many webgl rendering contexts?

This will be copy pasted all over the place so best to do it right now.


src/math/onehot_test.ts, line 63 at r1 (raw file):

    expect(res.shape).toEqual([4, 3]);
    test_util.expectArraysClose(res.getValues(), expected);
  });

add unit tests for non-0 and 1 on/off values


src/math/webgl/multinomial_gpu.ts, line 37 at r1 (raw file):

      uniform float seed;

      const vec2 K1 = vec2(

can you move this out to shader_compiler just like you did for round?


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

Review status: 2 of 9 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.


package.json, line 16 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

use a tilde for consistency?

nice catch. done


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

Previously, nsthorat (Nikhil Thorat) wrote…

why not just put the default here? onValue = 1, offValue = 0

ah, you're right. didn't occur to me for some reason. done


src/math/math_cpu.ts, line 1022 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

you need to fill this with offValue

nice catch. done


src/math/math_cpu.ts, line 1025 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

s/1/onValue

ups. rushed this. done. will add tests.


src/math/multinomial_test.ts, line 33 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Here, and in the other file, can you add an afterAll and dispose the math so we don't create too many webgl rendering contexts?

This will be copy pasted all over the place so best to do it right now.

nice. dispose() was missing in the common NDArrayMath class, but added now. The NDArrayMathGPU overwrites it to dispose the webgl resources.


src/math/onehot_test.ts, line 63 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

add unit tests for non-0 and 1 on/off values

Done.


src/math/webgl/multinomial_gpu.ts, line 37 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you move this out to shader_compiler just like you did for round?

Done.


Comments from Reviewable

@dsmilkov dsmilkov merged commit 5cb5951 into master Sep 30, 2017
@dsmilkov dsmilkov deleted the multinomial branch September 30, 2017 20:01
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
* add multinomial

* add cpu implementation and tests

* add onehot operation

* resolve comments
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