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

[BUGFIX] Fix MKLDNN BatchNorm with even number of channels (#19150)#19299

Merged
szha merged 3 commits intoapache:v1.7.xfrom
akarbown:batch_norm_is_view
Oct 25, 2020
Merged

[BUGFIX] Fix MKLDNN BatchNorm with even number of channels (#19150)#19299
szha merged 3 commits intoapache:v1.7.xfrom
akarbown:batch_norm_is_view

Conversation

@akarbown
Copy link
Contributor

@akarbown akarbown commented Oct 6, 2020 •
edited
Loading

Even number of channels results in data reordering before batch
norm operation. Therefore, if BatchNorm data array is view of
another array and the data is stored in MKLDNN format, the data
needs to be converted to the default format.

It fixes: #19150.

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented

@akarbown
Fix MKLDNN BatchNorm with even number of channels (apache#19150)
7abf990 
Even number of channels results in data reordering before batch
norm operation. Therefore, if BatchNorm data array is view of
another array and the data is stored in MKLDNN format, the data
needs to be converted to the default format.
@mxnet-bot
Copy link

mxnet-bot commented Oct 6, 2020

Hey @akarbown , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [website, centos-gpu, centos-cpu, miscellaneous, unix-cpu, windows-cpu, sanity, windows-gpu, edge, unix-gpu, clang]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@akarbown akarbown changed the title Fix MKLDNN BatchNorm with even number of channels (#19150) [BUGFIX] Fix MKLDNN BatchNorm with even number of channels (#19150) Oct 6, 2020
@akarbown
Copy link
Contributor Author

akarbown commented Oct 7, 2020

@mxnet-bot run ci [edge]

@mxnet-bot
Copy link

mxnet-bot commented Oct 7, 2020

Jenkins CI successfully triggered : [edge]

@akarbown
Copy link
Contributor Author

akarbown commented Oct 7, 2020

@mxnet-bot run ci [edge]

@mxnet-bot
Copy link

mxnet-bot commented Oct 7, 2020

Jenkins CI successfully triggered : [edge]

@akarbown
Copy link
Contributor Author

akarbown commented Oct 8, 2020

@mxnet-bot run ci [edge]

@mxnet-bot
Copy link

mxnet-bot commented Oct 8, 2020

Jenkins CI successfully triggered : [edge]

@akarbown
Copy link
Contributor Author

akarbown commented Oct 9, 2020

@PatricZhao , @TaoLv could you please review it!
This review is for branch 1.7, but the issue seems to occur on the other branches as well. Thus, it'll need to be applied on them also.
[With reference to the job 'edge' tests - it seems to be not related to this change.]

@TaoLv
Copy link
Member

TaoLv commented Oct 22, 2020

@akarbown please add the issue link to the description of this PR. Also do you want to add a unit test?

@wkcn could you please help to review?

@lanking520 lanking520 added pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-work-in-progress PR is still work in progress labels Oct 22, 2020
wkcn
wkcn reviewed Oct 22, 2020
View reviewed changes
Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

Thank you for the fix : )

Could you please update the test cases for MKLDNN-BatchNorm with even number of channels in tests/python/mkl/test_mkldnn.py#L297 ?

for shape in [(2, 3), (2, 4), (2, 3, 2, 2), (2, 4, 2, 2)]:

@lanking520 lanking520 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Oct 22, 2020
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-work-in-progress PR is still work in progress labels Oct 22, 2020
@akarbown
Copy link
Contributor Author

akarbown commented Oct 22, 2020

Thank you for the fix : )

Could you please update the test cases for MKLDNN-BatchNorm with even number of channels in tests/python/mkl/test_mkldnn.py#L297 ?

for shape in [(2, 3), (2, 4), (2, 3, 2, 2), (2, 4, 2, 2)]:

Done!
I've also added one more test similar to the one from the requested issue.

@lanking520 lanking520 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Oct 22, 2020
wkcn
wkcn approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Oct 23, 2020
@szha
Copy link
Member

szha commented Oct 25, 2020

thanks for the fix. please make sure to port to other branches (e.g. v1.x, v1.8.x, master) as appropriate.

@szha szha merged commit a22abce into apache:v1.7.x Oct 25, 2020
This was referenced Oct 26, 2020
Merged
Merged
Merged
samskalicky pushed a commit that referenced this pull request Oct 29, 2020
@akarbown
[v1.8.x][BUGFIX] Fix MKLDNN BatchNorm with even number of channels (#…
3e02554 
…19150) #19299 #19425 (#19428)

* Fix MKLDNN BatchNorm with even number of channels (#19150)

Even number of channels results in data reordering before batch
norm operation. Therefore, if BatchNorm data array is view of
another array and the data is stored in MKLDNN format, the data
needs to be converted to the default format.

* Add or updated test to verify Batchnorm odd & even number of channels

* Fix for Batchnorm odd & even chnls number context
samskalicky pushed a commit that referenced this pull request Oct 29, 2020
@akarbown
[v1.x][BUGFIX]Fix MKLDNN BatchNorm with even number of channels (#19150…
7e85232 
…) #19299 #19425 (#19445)

* Fix MKLDNN BatchNorm with even number of channels (#19150)

Even number of channels results in data reordering before batch
norm operation. Therefore, if BatchNorm data array is view of
another array and the data is stored in MKLDNN format, the data
needs to be converted to the default format.

* Add or updated test to verify Batchnorm odd & even number of channels

* Fix for Batchnorm odd & even chnls number context
samskalicky pushed a commit that referenced this pull request Oct 30, 2020
@akarbown
[BUGFIX] Fix MKLDNN BatchNorm with even number of channels (#19150) #…
0faecf0 
…19299 (#19425)

* Fix MKLDNN BatchNorm with even number of channels (#19150)

Even number of channels results in data reordering before batch
norm operation. Therefore, if BatchNorm data array is view of
another array and the data is stored in MKLDNN format, the data
needs to be converted to the default format.

* Add or updated test to verify Batchnorm odd & even number of channels

* Fix for Batchnorm odd & even chnls number context
@samskalicky samskalicky mentioned this pull request Nov 9, 2020
Closed
vidyaravipati pushed a commit to vidyaravipati/incubator-mxnet that referenced this pull request Nov 11, 2020
@akarbown
[BUGFIX] Fix MKLDNN BatchNorm with even number of channels (apache#19150
406157b 
) apache#19299 (apache#19425)

* Fix MKLDNN BatchNorm with even number of channels (apache#19150)

Even number of channels results in data reordering before batch
norm operation. Therefore, if BatchNorm data array is view of
another array and the data is stored in MKLDNN format, the data
needs to be converted to the default format.

* Add or updated test to verify Batchnorm odd & even number of channels

* Fix for Batchnorm odd & even chnls number context
@akarbown akarbown deleted the batch_norm_is_view branch November 16, 2020 10:25
chinakook pushed a commit to chinakook/mxnet that referenced this pull request Nov 17, 2020
@akarbown @chinakook
[BUGFIX] Fix MKLDNN BatchNorm with even number of channels (apache#19150
5d84992 
) apache#19299 (apache#19425)

* Fix MKLDNN BatchNorm with even number of channels (apache#19150)

Even number of channels results in data reordering before batch
norm operation. Therefore, if BatchNorm data array is view of
another array and the data is stored in MKLDNN format, the data
needs to be converted to the default format.

* Add or updated test to verify Batchnorm odd & even number of channels

* Fix for Batchnorm odd & even chnls number context
chinakook pushed a commit to chinakook/mxnet that referenced this pull request Nov 19, 2020
@akarbown @chinakook
[BUGFIX] Fix MKLDNN BatchNorm with even number of channels (apache#19150
1b1761f 
) apache#19299 (apache#19425)

* Fix MKLDNN BatchNorm with even number of channels (apache#19150)

Even number of channels results in data reordering before batch
norm operation. Therefore, if BatchNorm data array is view of
another array and the data is stored in MKLDNN format, the data
needs to be converted to the default format.

* Add or updated test to verify Batchnorm odd & even number of channels

* Fix for Batchnorm odd & even chnls number context
josephevans pushed a commit to josephevans/mxnet that referenced this pull request Dec 8, 2020
@akarbown
[v1.x][BUGFIX]Fix MKLDNN BatchNorm with even number of channels (apac…
9964639 
…he#19150) apache#19299 apache#19425 (apache#19445)

* Fix MKLDNN BatchNorm with even number of channels (apache#19150)

Even number of channels results in data reordering before batch
norm operation. Therefore, if BatchNorm data array is view of
another array and the data is stored in MKLDNN format, the data
needs to be converted to the default format.

* Add or updated test to verify Batchnorm odd & even number of channels

* Fix for Batchnorm odd & even chnls number context
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@wkcn wkcn wkcn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-awaiting-review PR is waiting for code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@akarbown @mxnet-bot @TaoLv @szha @wkcn @lanking520