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

Conversation

dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Aug 18, 2017

This resolves #18

This change is Reviewable

@dsmilkov dsmilkov changed the title Remove math.reshape Move the rest of the math ops to logical Aug 21, 2017
@dsmilkov dsmilkov requested a review from nsthorat August 21, 2017 01:52
@nsthorat
Copy link
Contributor

:lgtm_strong:


Reviewed 6 of 7 files at r1, 17 of 18 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


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

    const program = new Copy2DProgram(sourceSizeRowCol[1], destSizeRowCol[1]);
    const customSetup = copy_gpu.getCustomSetupFunc(
        sourceBeginRowCol, destBeginRowCol, destSizeRowCol);

one thought: getCustomSetupFunc could be part of the GPGPUProgram, though you lose a little flexibility. I'm fine either way, just throwing ideas out there :)


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

      alignCorners: boolean): Array3D {
    const program =
        new ResizeBilinearProgram(x.shape, newShape2D, alignCorners);

rename this to ResizeBilinear3DProgram


src/math/webgl/batchnorm_gpu_test.ts, line 130 at r2 (raw file):

    const result = uploadBatchNormDownload(
        x, [2, 3, 3], mean, [1, 3], variance, [1, 3], offset, [1, 3], scale,

does 2, 9 not work, or do you not like that test?


src/math/webgl/resize_bilinear_gpu.ts, line 42 at r2 (raw file):

      const vec2 effectiveInputOverOutputRatioRC = vec2(
          ${effectiveInputShape[0] /
        effectiveOutputShape[0]},

i think this fits on prev line


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

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


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

Previously, nsthorat (Nikhil Thorat) wrote…

one thought: getCustomSetupFunc could be part of the GPGPUProgram, though you lose a little flexibility. I'm fine either way, just throwing ideas out there :)

Done.


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

Previously, nsthorat (Nikhil Thorat) wrote…

rename this to ResizeBilinear3DProgram

Done.


src/math/webgl/batchnorm_gpu_test.ts, line 130 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

does 2, 9 not work, or do you not like that test?

[2, 9] corresponds to the texture shape, which is what the old uploadBatchNormDownload was expecting. [2, 3, 3] is the logical shape which is what the rewritten uploadDownload is expecting. (I didn't want any dependency/usage of texture shapes in the tests)


src/math/webgl/resize_bilinear_gpu.ts, line 42 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

i think this fits on prev line

Done.


Comments from Reviewable

@dsmilkov dsmilkov merged commit 156c227 into master Aug 21, 2017
@dsmilkov dsmilkov deleted the reshape branch August 21, 2017 15:32
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
* migrate addScaledMat and conv2d to logical sampling and improve shader compiler

* fix conv2d zero paddig and make the project build

* migrate rest of conv shaders to logical

* replace zero pad with if

* migrate pool ops

* removing math.reshape

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

* migrate copy op to logical

* remove duplicate copy file

* move the rest of math ops to logical

* address 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