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

Change conv2d to match TF API #67

Merged
merged 22 commits into from
Sep 7, 2017
Merged

Change conv2d to match TF API #67

merged 22 commits into from
Sep 7, 2017

Conversation

dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Aug 25, 2017

This fixes #106

This change is Reviewable

@dsmilkov dsmilkov requested a review from nsthorat September 6, 2017 17:00
@nsthorat
Copy link
Contributor

nsthorat commented Sep 6, 2017

:lgtm_strong:


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


src/math/conv_util.ts, line 23 at r2 (raw file):

};

export function computeOutputInfo(

can you write a couple unit tests for this? it's a well defined function


src/math/math.ts, line 901 at r2 (raw file):

   * @param bias Optional bias, rank 1 of shape [outDepth].
   * @param strides The strides of the convolution: [strideHeight, strideWidth].
   * @param pad A string from: 'same', 'valid'. The type of padding algorithm.

or a number.

Can you briefly explain what SAME | VALID actually means? the naming is confusing to begin with, but a simple jsdoc here would be really nice


src/math/math.ts, line 1407 at r2 (raw file):

  let param2: number;
  if (typeof param === 'number') {
    param1 = param;

why not shorten this whole thing

if (typeof param === 'number') { return [param, param]; } return param;


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

dsmilkov commented Sep 7, 2017

Reviewed 1 of 12 files at r1.
Review status: 1 of 26 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/math/conv_util.ts, line 23 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you write a couple unit tests for this? it's a well defined function

done.


src/math/math.ts, line 901 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

or a number.

Can you briefly explain what SAME | VALID actually means? the naming is confusing to begin with, but a simple jsdoc here would be really nice

Done.


src/math/math.ts, line 1407 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

why not shorten this whole thing

if (typeof param === 'number') { return [param, param]; } return param;

oh jeez ups :) made it one-liner. I can't program simple things anymore. only glsl...


Comments from Reviewable

@dsmilkov dsmilkov merged commit cbd08fe into master Sep 7, 2017
@dsmilkov dsmilkov deleted the conv branch September 7, 2017 19:53
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
* update the readme and npm package

* update readme

* change conv2d api to take padding same|valid

* Merge branch 'master' into conv

* move logic to math

* some progress

* Merge remote-tracking branch 'origin/master' into conv

* Migrate all conv-related ops

* rename conv2dTranspose to conv2dDerInput

* switch shader indexing from float to int

* revert graph_runner_test

* self review

* Merge remote-tracking branch 'origin/master' into conv

* merge with the branch int_indexing

* fix typos in shaders

* merge with master

* update pool ops

* remove commented out code

* simplify api

* added unit tests for conv/pool

* add doc

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