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

[ARITH] cleanup the indexmod/div on python side #4028

Merged
merged 5 commits into from
Sep 28, 2019
Merged

Conversation

tqchen
Copy link
Member

@tqchen tqchen commented Sep 27, 2019

No description provided.

@tqchen tqchen force-pushed the arithx branch 5 times, most recently from 0c341e4 to 700bac9 Compare September 27, 2019 21:07
@tqchen
Copy link
Member Author

tqchen commented Sep 27, 2019

@kevinthesun @icemelon9 please check the changes in the multi-box prior. The error was detected when performing an integer division that has ambiguity, but seems we should have do float div.

@tqchen tqchen force-pushed the arithx branch 2 times, most recently from 2710880 to ef69736 Compare September 27, 2019 22:04
@kevinthesun
Copy link
Contributor

@kevinthesun @icemelon9 please check the changes in the multi-box prior. The error was detected when performing an integer division that has ambiguity, but seems we should have do float div.

@tqchen Do you mean we need to explicitly cast size[i] * height to avoid such error?

@tqchen
Copy link
Member Author

tqchen commented Sep 28, 2019

just to confirm what is the intend of the code. I changed the code to explicitly cast to float. Otherwise it will do integer division(the result will be rounded)

@tqchen
Copy link
Member Author

tqchen commented Sep 28, 2019

@kevinthesun to elaborate the further, previously the code was

ratio_int * height / width / 2.0 

Originally because both ratio_int * height and width have int type, the division will be translated to integer division(which truncates). Imagine ratio_int = 2, height=10, width = 20, the result of ratio_int * height / width will be 2 * 10 / 20 = 1.

The current set of change detects this division and raises an error, so we can explicit do the cast which will give the floating pt division.

float32(ratio_int * height) / width

I assumed that the floating pt division is what we want here and we just revealed a previous bug in the code. but it would be great if you can also help confirm that also cc @icemelon9

@tqchen tqchen merged commit f98035b into apache:master Sep 28, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 30, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 30, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Oct 1, 2019
petrex added a commit to petrex/tvm that referenced this pull request Oct 29, 2019
* master:
  Fix split's last factor issue (apache#4044)
  [COMMUNITY] ajtulloch -> committer (apache#4043)
  [TOPI]Add op argwhere (apache#3994)
  [topi] add ARM v8.2 udot (uint8) support (apache#3978)
  [COMMUNITY] anijain2305 -> reviewer (apache#4036)
  [QNN] Renaming dense operator. (apache#4033)
  [Relay][Compile_engine] Int64 shape handling for outputs. (apache#4031)
  Add dmlc-core to the list of installed header directories. (apache#4035)
  [ARITH] migrate indexdiv/mod to floordiv/mod (apache#4008)
  [Relay] Move prelude to text format (apache#3939)
  make tvm compilable by gcc 4.9.2 (apache#4032)
  [AUTOTVM][DOCS] Add a link to the defining network description of auto-tuning tutorial (apache#4023)
  [ARITH] cleanup the indexmod/div on python side (apache#4028)
  [Fix] Add more pad_mode support for onnx converter (apache#4029)
  Add parser support for ReLU tflite operator (apache#4022)
  Additional MXNet Convolution and Deconvolution tests (apache#4026)
  docs: minor spelling tweaks (apache#4027)
@tqchen tqchen deleted the arithx branch December 23, 2019 23:41
tqchen added a commit to tqchen/tvm that referenced this pull request Mar 29, 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

Successfully merging this pull request may close these issues.

2 participants