Revise implementation of BatchNormalization training code to align with tf.keras

[caisq]

• The implementation of the training algorithm for BatchNormalization is different between keras-team/keras and tensorflow.keras.
• This PR aligns the implementation with the latter. (Previously it aligned with the former.) Rationale:
  • Alignment with TensorFlow is a higher priority for TensorFlow.js than alignment with keras-team/keras
  • The convergence on the MNIST-ACGAN example is better with the tf.keras implementation.
• While this change has no implication for the public API of tf.layers.batchNormalization, it may be breaking for users who rely on the exact numeric values of the batchNormalization layer during training.

BREAKING

This change is 

[bileschi]

Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @bileschi)

---

src/layers/normalization.ts, line 20 at r1 (raw file):

import {Constraint, ConstraintIdentifier, getConstraint, serializeConstraint} from '../constraints';
import {InputSpec, Layer, LayerConfig} from '../engine/topology';
import {getScalar} from '../backend/state';

Why did this move?

[nsthorat]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @caisq)

---

src/layers/normalization.ts, line 398 at r1 (raw file):

          (variable: LayerVariable, value: Tensor, momentum: number): void => {
            tfc.tidy(() => {
              const decay = getScalar(1.0).sub(getScalar(momentum));

FYI you should no longer need this scalar cache. You can also just pass momentum directly to sub (no need for wrapping in a scalar tensor).

[bileschi]

cool!

…

On Fri, Nov 30, 2018 at 3:08 PM Nikhil Thorat ***@***.***> wrote: ***@***.**** commented on this pull request. *Reviewable <https://reviewable.io/reviews/tensorflow/tfjs-layers/385>* status: [image: ] complete! 1 of 1 approvals obtained (waiting on @caisq <https://github.com/caisq>) ------------------------------ *src/layers/normalization.ts, line 398 at r1 <https://reviewable.io/reviews/tensorflow/tfjs-layers/385#-LSaDu0cDJI0esdWFPGM:-LSaDu0cDJI0esdWFPGN:bfitgcf> (raw file <https://github.com/tensorflow/tfjs-layers/blob/2c27de3b9ac10c2d4a724e263d98255b27e4ca79/src/layers/normalization.ts#L398>):* (variable: LayerVariable, value: Tensor, momentum: number): void => { tfc.tidy(() => { const decay = getScalar(1.0).sub(getScalar(momentum)); FYI you should no longer need this scalar cache. You can also just pass momentum directly to sub (no need for wrapping in a scalar tensor). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#385 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAhZTmzjKkkVQQ9zFM0Djmt8UzIvI6euks5u0ZAqgaJpZM4Y8PPh> .

-- Stan Bileschi Ph.D. | SWE | bileschi@google.com | 617-230-8081

[caisq]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @bileschi)

---

src/layers/normalization.ts, line 20 at r1 (raw file):

Previously, bileschi (Stanley Bileschi) wrote…

Why did this move?

So that the imports are sorted alphabetically by the source file path.

---

src/layers/normalization.ts, line 398 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

FYI you should no longer need this scalar cache. You can also just pass momentum directly to sub (no need for wrapping in a scalar tensor).

Ack. Thanks. There are a lot of places like this that should be fixed in a batch. Filed an issue to track this.
tensorflow/tfjs#957