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

[Relay][FrontEnd][MxNet] incorrect data_min_idx and data_max_idx in _qnn_conv #4831

Closed
GuoliangLiCN opened this issue Feb 6, 2020 · 8 comments

Comments

@GuoliangLiCN
Copy link

In the lines 1409-1413 of file python/tvm/relay/frontend/mxnet.py, are there typos when has_sum is False?

        data_min_idx = -1
        data_max_idx = -2
        if has_sum:
            data_min_idx = -4
            data_max_idx = -3

Should data_min_idx be -2 and data_max_idx be -1 according to lines 467-474 of https://github.com/apache/incubator-mxnet/blob/master/src/operator/subgraph/mkldnn/mkldnn_conv.cc

if (param.full_conv_param.mkldnn_param.quantized) {
input_names.emplace_back("data_min");
input_names.emplace_back("data_max");
if (param.full_conv_param.mkldnn_param.with_sum) {
input_names.emplace_back("sum_min");
input_names.emplace_back("sum_max");
}
}

@tqchen
Copy link
Member

tqchen commented Feb 26, 2020

cc @kevinthesun @zhiics can you help check on this ?

@zhiics
Copy link
Member

zhiics commented Feb 27, 2020

yeah, it looks flipped to me as well. @shoubhik can you confirm? Thanks.

@shoubhik
Copy link
Contributor

taking a look at it.

@anijain2305
Copy link
Contributor

Thanks for finding the bug @GuoliangLiCN
Yeah, it is flipped. The bug hid because the scale and zero point remained same even if the data_min and data_max were flipped.

@shoubhik
Copy link
Contributor

yes, it does needs a flipping. the test case passes because the scale value remains the same. I'll send a PR a little later.

@tqchen
Copy link
Member

tqchen commented Mar 20, 2020

What is the state of this issue? @anijain2305 @shoubhik

@shoubhik
Copy link
Contributor

sorry got swamped. I will definitely send a fix on Monday.

shoubhik added a commit to shoubhik/incubator-tvm that referenced this issue Mar 23, 2020
…pped.

Existing test cases cover this fix. In addition I have added an assert to make sure that the data_min is always less than equal to data_max.
tqchen pushed a commit that referenced this issue Mar 24, 2020
…5136)

Existing test cases cover this fix. In addition I have added an assert to make sure that the data_min is always less than equal to data_max.
@tqchen
Copy link
Member

tqchen commented Mar 24, 2020

#5136

@tqchen tqchen closed this as completed Mar 24, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this issue Apr 16, 2020
…pped. (apache#5136)

Existing test cases cover this fix. In addition I have added an assert to make sure that the data_min is always less than equal to data_max.
zhiics pushed a commit to neo-ai/tvm that referenced this issue Apr 17, 2020
…pped. (apache#5136)

Existing test cases cover this fix. In addition I have added an assert to make sure that the data_min is always less than equal to data_max.
shoubhik added a commit to shoubhik/incubator-tvm that referenced this issue May 12, 2020
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

5 participants