Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TOPI] topi.nn.batch_norm_inference is broken #2470

Closed
headupinclouds opened this issue Jan 20, 2019 · 5 comments
Closed

[TOPI] topi.nn.batch_norm_inference is broken #2470

headupinclouds opened this issue Jan 20, 2019 · 5 comments

Comments

@headupinclouds
Copy link
Contributor

The topi.nn.batch_norm_inference python implementation appears to be broken.

  File "/dl/mxnet/3rdparty/tvm/topi/python/topi/nn/batch_norm.py", line 54, in batch_norm_inference
    mean = tvm.compute((C, ), lambda c: moving_mean[c])
NameError: name 'C' is not defined

https://github.com/dmlc/tvm/blob/e4b9f986dab8c48ba109a52106565fc4be6b67c4/topi/python/topi/nn/batch_norm.py#L54-L56

Should the offending lines (mean and var return variables) just be removed here?

C++ prototype:

https://github.com/dmlc/tvm/blob/e4b9f986dab8c48ba109a52106565fc4be6b67c4/topi/include/topi/nn/batch_norm.h#L33-L41

For what it's worth, topi.nn.batch_norm_inference doesn't currently have a unit test, and I don't see any other references to it in the repository:

find /dl/mxnet/3rdparty/tvm/ -name "*.py" | xargs grep batch_norm_inference
/dl/mxnet/3rdparty/tvm/topi/python/topi/nn/batch_norm.py:def batch_norm_inference(data, gamma, beta, moving_mean, moving_var, eps, fix_gamma):
@headupinclouds headupinclouds changed the title topi.nn.batch_norm_inference is broken [TOPI] topi.nn.batch_norm_inference is broken Jan 20, 2019
@yzhliu
Copy link
Member

yzhliu commented Jan 21, 2019

I feel batch_norm_inference is no longer used as batch_norm gets decomposed into several scalar operations in simplify_inference pass.

@headupinclouds
Copy link
Contributor Author

I feel batch_norm_inference is no longer used as batch_norm gets decomposed into several scalar operations in simplify_inference pass

I see. Are you suggesting it should just be removed?

@yzhliu
Copy link
Member

yzhliu commented Jan 22, 2019

Yes, let's do it. would you mind shoot a PR to remove?

@headupinclouds
Copy link
Contributor Author

Sure, I can put together a PR removing both
tvm/topi/include/topi/nn/batch_norm.h,
tvm/topi/python/topi/nn/batch_norm.py, and any references.

headupinclouds added a commit to headupinclouds/tvm that referenced this issue Feb 19, 2019
* rm topi/include/topi/nn/batch_norm.h
* rm topi/python/topi/nn/batch_norm.py
* rm `#include <top/nn/batch_norm.h>` in topi/src/topi.cc
* rm `from .batch_norm import *` in topi/python/topi/nn/__init__.py

see issue apache#2470

> `batch_norm_inference` is no longer used as `batch_norm` gets decomposed into several scalar operations in `simplify_inference` pass
@headupinclouds
Copy link
Contributor Author

pushed #2626

@tqchen tqchen closed this as completed Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants