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

[AUTOTVM] Fix a bug in generating the search space #4779

Merged
merged 1 commit into from Jan 29, 2020

Conversation

wpan11nv
Copy link
Contributor

  • Do not use numpy.prod which ignores integer (64 bits) overflows.
    This leads to an incorrect number of points in the search space.

Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Never notice this issue before. Good catch.

@@ -226,7 +226,13 @@ def __init__(self, axes, policy, **kwargs):
def _generate_space(self, now, tmp_stack, enforce_no_tail=False):
"""Generate space by DFS"""
if now == self.num_output - 1:
prod = np.prod(tmp_stack, dtype=np.int64)
prod = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that manually implementing a classic array production is not necessary in any case. Since limited types are only enforced in numpy and Python's types are unlimited, it would be more concise to use Python builtins to calculate the product:

import functools
import operator
prod = functools.reduce(operator.mul, tmp_stack, 1)

Note that the length of tmp_stack is always small (currently 4 at most, and I don't think it would be longer than 10), so this won't hurt the performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks more pythonic. The number of lines will be similar. Here is one debate on this: :)

https://stackoverflow.com/questions/9474412/python-alternative-to-reduce

Copy link
Contributor

@comaniac comaniac Jan 27, 2020

Choose a reason for hiding this comment

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

Well, I know that some people prevent reduce and other patterns like map from being used in Python 3, although I personally think not like map and filter, reduce is hard to be replaced by other pythonic representations. I am fine with that if you do not want to introduce reduce to the code. In this case I would suggest making this logic a standalone function in autotvm/util.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Utility function looks like a overkill. A reduce call is fine. Updated. Thanks!

- Do not use numpy.prod which ignores integer (64 bits) overflows.
  This leads to an incorrect number of points in the search space.
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM

@wpan11nv
Copy link
Contributor Author

Please help review this patch.

@merrymercy merrymercy merged commit 1b8522e into apache:master Jan 29, 2020
@wpan11nv wpan11nv deleted the autotvm_overflow branch January 29, 2020 17:42
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 26, 2020
- Do not use numpy.prod which ignores integer (64 bits) overflows.
  This leads to an incorrect number of points in the search space.
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 28, 2020
- Do not use numpy.prod which ignores integer (64 bits) overflows.
  This leads to an incorrect number of points in the search space.
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2020
- Do not use numpy.prod which ignores integer (64 bits) overflows.
  This leads to an incorrect number of points in the search space.
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.

None yet

3 participants