Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Mar 30, 2020

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@lina128 lina128 requested review from annxingyuan, dsmilkov and tafsiri and removed request for dsmilkov March 30, 2020 19:30
@lina128 lina128 changed the title Modularize batchnorm. [WIP] Modularize batchnorm. Mar 30, 2020
@lina128 lina128 requested review from annxingyuan, dsmilkov and tafsiri and removed request for annxingyuan and dsmilkov March 31, 2020 01:02
@lina128 lina128 changed the title [WIP] Modularize batchnorm. [core] Modularize batchnorm. Mar 31, 2020
Copy link
Contributor

@tafsiri tafsiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! LGTM with three very tiny comments.

Reviewed 13 of 13 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, and @tafsiri)


tfjs-core/src/kernel_names.ts, line 24 at r1 (raw file):

import {PixelData} from './types';

export const BatchNormalization = 'BatchNormalization';

I believe your observation about this mapping onto FusedBatchNorm is correct so let's use that as the kernel name. In the converter all versions of FusedBatchNorm mapped onto this function https://cs.opensource.google/tensorflow/tfjs/+/master:tfjs-converter/src/operations/executors/normalization_executor.ts;l=31?q=batchnorm&ss=tensorflow%2Ftfjs:tfjs-converter%2F


tfjs-core/src/ops/batchnorm.ts, line 102 at r1 (raw file):

          'equal ranks.');

  const x4D: Tensor4D = xAs4D($x);

Let's move this into the forward function.


tfjs-core/src/ops/batchnorm4d.ts, line 93 at r1 (raw file):

}

export const batchNormalization4d = op({batchNormalization4d_});

Could you add a //todo(yassogba) to remove these later since the batchNormalization* form is already deprecated.

@lina128
Copy link
Collaborator Author

lina128 commented Mar 31, 2020

Great work! LGTM with three very tiny comments.

Reviewed 13 of 13 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, and @tafsiri)

tfjs-core/src/kernel_names.ts, line 24 at r1 (raw file):

import {PixelData} from './types';

export const BatchNormalization = 'BatchNormalization';

I believe your observation about this mapping onto FusedBatchNorm is correct so let's use that as the kernel name. In the converter all versions of FusedBatchNorm mapped onto this function https://cs.opensource.google/tensorflow/tfjs/+/master:tfjs-converter/src/operations/executors/normalization_executor.ts;l=31?q=batchnorm&ss=tensorflow%2Ftfjs:tfjs-converter%2F

tfjs-core/src/ops/batchnorm.ts, line 102 at r1 (raw file):

          'equal ranks.');

  const x4D: Tensor4D = xAs4D($x);

Let's move this into the forward function.

tfjs-core/src/ops/batchnorm4d.ts, line 93 at r1 (raw file):

}

export const batchNormalization4d = op({batchNormalization4d_});

Could you add a //todo(yassogba) to remove these later since the batchNormalization* form is already deprecated.

Thank you for the review, Yannick! Done. Also changed all the ops to be called directly instead of chained through Tensor in grad.

@tafsiri
Copy link
Contributor

tafsiri commented Apr 1, 2020

Great! transforming away from chained ops is great to do when we can.

@lina128 lina128 merged commit 707d30d into tensorflow:master Apr 1, 2020
@lina128 lina128 deleted the batchnorm branch April 30, 2020 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants