-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
There was a problem hiding this 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.
python/tvm/autotvm/task/space.py
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
75bd49f
to
a1c85dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please help review this patch. |
- Do not use numpy.prod which ignores integer (64 bits) overflows. This leads to an incorrect number of points in the search space.
- Do not use numpy.prod which ignores integer (64 bits) overflows. This leads to an incorrect number of points in the search space.
- Do not use numpy.prod which ignores integer (64 bits) overflows. This leads to an incorrect number of points in the search space.
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.