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

Align API with TF #956

Merged
merged 6 commits into from Apr 16, 2018
Merged

Align API with TF #956

merged 6 commits into from Apr 16, 2018

Conversation

dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Apr 15, 2018

Aligns the backend API and functionality (NaN propagation, dtype strictness, kernel signatures) with TensorFlow Python.

  • Remove backend.minPool since TF doesn't have it.
  • Remove normRegion param in localResponseNormalization kernel since TF doesn't support it.
  • Remove leakyRelu, prelu and preluDer from the backend and implement using higher-level ops, aligning with TF Python.
  • Make backend.multinomial take logits instead of probabilities and normalized: boolean param for backwards compatibility.
  • argMin and argMax take single axis: number instead of axes: number[]
  • Change eluDer(x: T): T signature to eluDer(dy: T, y: T): T to align with TF.
  • Change NaN behavior of max/avgPool and conv2d to align with TF.
  • Change avgPool out of bounds (padding) behavior to align with TF
  • Require indices in oneHot and gather to be of dtype int32

Fixes tensorflow/tfjs#195


This change is Reviewable

@manrajgrover
Copy link
Contributor

manrajgrover commented Apr 15, 2018

@dsmilkov I just saw this. Would you be fixing tensorflow/tfjs#195 in this PR?

@dsmilkov
Copy link
Contributor Author

Nice observation! Yes, those will be removed as part of that PR

@dsmilkov dsmilkov requested a review from nsthorat April 16, 2018 03:20
@dsmilkov dsmilkov changed the title [WIP] Align API with TF Align API with TF Apr 16, 2018
@nsthorat
Copy link
Contributor

:lgtm_strong:


Reviewed 9 of 18 files at r1, 8 of 14 files at r2.
Review status: 17 of 24 files reviewed at latest revision, all discussions resolved, some commit checks failed.


src/index.ts, line 68 at r2 (raw file):

  gpgpu_util
};
export {WebGLTimingInfo} from './kernels/backend_webgl';

put this under webgl


src/kernels/webgl/pool_gpu.ts, line 108 at r2 (raw file):

        'minMaxValue[0], minMaxValue[1]), minMaxValue[2]), minMaxValue[3])';
    if (poolType === 'avg') {
      returnValue = `avgValue / count`;

why keep an accumulator around when you know this statically?


src/ops/lrn.ts, line 44 at r2 (raw file):

  static localResponseNormalization<T extends Tensor3D|Tensor4D>(
      x: T, radius = 5, bias = 1, alpha = 1, beta = 0.5,
      normRegion: 'acrossChannels'|'withinChannel' = 'acrossChannels'): T {

this will break caffe

can you just throw for the TF backend? I'd like to not break API if possible. Going forward we can reject these changes.


src/ops/multinomial_test.ts, line 31 at r2 (raw file):

    const probs = tf.tensor1d([0.5, 0.5]);
    const seed: number = null;
    const normalized = true;

the API still allows normalized, but we now don't have coverage


src/ops/pool.ts, line 165 at r2 (raw file):

  @doc({heading: 'Operations', subheading: 'Convolution'})
  @operation
  static minPool<T extends Tensor3D|Tensor4D>(

hmm... I depend on this for some channel-normalization stuff in the activation visualization demos. What about just throwing in the backend? Alternatively, we could think about finding a way to expose the WebGL backend directly so we can call a WebGL kernel that does this.


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

Thanks for the fast review!


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


src/index.ts, line 68 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

put this under webgl

Can't do it since it's a type.


src/kernels/webgl/pool_gpu.ts, line 108 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

why keep an accumulator around when you know this statically?

to align with TF avgPool behavior where padded values are ignored (different than treating them as 0)


src/ops/lrn.ts, line 44 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this will break caffe

can you just throw for the TF backend? I'd like to not break API if possible. Going forward we can reject these changes.

Chatted offline. LRN is rarely used and we are keeping the default behavior.


src/ops/multinomial_test.ts, line 31 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

the API still allows normalized, but we now don't have coverage

Chatted offline. We have webgl and cpu coverage, and there is something we can do later on for other backends.


src/ops/pool.ts, line 165 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

hmm... I depend on this for some channel-normalization stuff in the activation visualization demos. What about just throwing in the backend? Alternatively, we could think about finding a way to expose the WebGL backend directly so we can call a WebGL kernel that does this.

We can use -maxPool(-x) to simulate minPool


Comments from Reviewable

@dsmilkov dsmilkov merged commit 7f63757 into master Apr 16, 2018
@dsmilkov dsmilkov deleted the align branch April 16, 2018 18:53
dsmilkov added a commit to tensorflow/tfjs-node that referenced this pull request Apr 16, 2018
**NOTE: Depends on tensorflow/tfjs-core#956

- Implement: `argMin`, `argMax`, `logicalXor`, `log1p`, `eluDer`, `clip`, `mod`, `round`, `sign`, `rsqrt`, `reciprocal`, `asinh`, `acosh`, `atanh`, `squaredDifference`, `expm1`, `atan2`, `conv2d`, `conv2dDerInput`, `conv2dDerFilter`, `maxPool`, `maxPoolBackprop`, `avgPool`, `avgPoolBackprop`, `tile`, `resizeBilinear`, `batchNorm`, `LRN`,  `multinomial`, `softplus`.
- Implement `depthwiseConv2D` for `dilations=1`. `dilations>1` requires `spaceToBatch` to be added to core
- Implement the backend methods: `time`, `memory`.
- Add binding functionality for types of attributes: `float` and `list(int)`
- Test the ops using `tfjs-core` tests
- Throw error if
  - padding type is `number` since TF backend doesn't have kernels for that - only `valid` and `same` is supported. We support padding type `number` in the webgl and cpu backend.
  - multinomial gets called with normalized probabilities. TF backend only supports logits (and you can't undo softmax), while we support both logits and probabilities in the webgl and cpu backend.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants