Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Test/mkldnn batch norm op #13084

Closed
wants to merge 183 commits into from
Closed

Conversation

azai91
Copy link
Contributor

@azai91 azai91 commented Nov 1, 2018

Description

Add tests for MKLDNN batch norm operator The creates ndarray inputs and then runs it separately through the native CPU BN operator and MKLDNN BN operator. Creating the BN test requires a different helper as the BN operation writes to the input, so identical arrays of equal value must be created and then ran through the native / MKLDNN operators.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Add tests for MKLDNN BN operator

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@azai91
Copy link
Contributor Author

azai91 commented Dec 11, 2018

@TaoLv please review

@Vikas-kum
Copy link
Contributor

@azai91 It will be helpful if you can please add more details to description explaining why tests where failing earlier, and how did you fix it.

@lupesko
Copy link
Contributor

lupesko commented Dec 11, 2018

@mseth10 @anirudh2290 pulling you guys in for a review...

@azai91
Copy link
Contributor Author

azai91 commented Dec 12, 2018

@Vikas89 added

@TaoLv
Copy link
Member

TaoLv commented Dec 12, 2018

Is it possible to separate operator change and unit test into two PRs?

@azai91
Copy link
Contributor Author

azai91 commented Dec 12, 2018

@TaoLv #13625

@TaoLv TaoLv mentioned this pull request Dec 12, 2018
7 tasks
bool inputs_valid = SupportMKLDNN(inputs[0]);
for (size_t i = 1; i < inputs.size(); i++) {
if (inputs[i].IsMKLDNNData()) {
inputs_valid = false;
Copy link
Member

Choose a reason for hiding this comment

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

inputs valid is set to false if it is in MKLDNN format ?

&& param.axis == mxnet::op::batchnorm::DEFAULT_AXIS
&& shape[param.axis] % 8 == 0
&& !mxnet::op::batchnorm::disable_mkl;
bool inputs_valid = SupportMKLDNN(inputs[0]);
for (size_t i = 1; i < inputs.size(); i++) {
if (inputs[i].IsMKLDNNData()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Once inputs_valid is set to false, we need not check remaining inputs, we can add a break here.
if (!inputs_valid) break;

@mseth10
Copy link
Contributor

mseth10 commented Dec 13, 2018

@azai91 can you please resolve merge conflicts.

@sandeep-krishnamurthy
Copy link
Contributor

@azai91 - can you please rebase?

@anirudhacharya
Copy link
Member

@azai91 please rebase and resolve merge conflicts

@azai91
Copy link
Contributor Author

azai91 commented Jan 14, 2019

closing since this is a duplicate of #13625

@azai91 azai91 closed this Jan 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
MKLDNN pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.