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] Fix reduce axis bug #3422

Merged
merged 2 commits into from
Jun 27, 2019
Merged

[Relay] Fix reduce axis bug #3422

merged 2 commits into from
Jun 27, 2019

Conversation

altanh
Copy link
Contributor

@altanh altanh commented Jun 23, 2019

Fixes failure of sum and all on axis=0. Added regression tests as well.

@altanh
Copy link
Contributor Author

altanh commented Jun 23, 2019

cc @siju-samuel @jroesch

@altanh
Copy link
Contributor Author

altanh commented Jun 23, 2019

It looks like this change was undone in the past for #2439, but I can't see how axis and isinstance(axis, int) differs from simply isinstance(axis, int) other than evaluating to false when axis=0, since isinstance(None, int) == False.

@jroesch
Copy link
Member

jroesch commented Jun 25, 2019

We need to prevent against the axis being None, it should be axis is not None and ... what happens if you pass None right now?

@altanh
Copy link
Contributor Author

altanh commented Jun 27, 2019

Not quite sure I understand, unless the existing code was incorrect in keeping None:

>>> axis = None
>>> axis = [axis] if axis and isinstance(axis, int) else axis
>>> axis is None
True

For clarity, this demonstrates the bug this PR fixes:

>>> axis = 0
>>> axis = [axis] if axis and isinstance(axis, int) else axis
>>> axis == [0]
False

whereas with the fix:

>>> axis = 0
>>> axis = [axis] if isinstance(axis, int) else axis
>>> axis == [0]
True
>>> axis = None
>>> axis = [axis] if isinstance(axis, int) else axis
>>> axis is None
True

Otherwise TVM crashes since it expects an array of ints or None, from what I can tell.

@tqchen tqchen merged commit 1e9d014 into apache:master Jun 27, 2019
@tqchen
Copy link
Member

tqchen commented Jun 27, 2019

THanks, @altanh @jroesch, this PR is now merged

wweic pushed a commit to wweic/tvm that referenced this pull request Jun 28, 2019
* fix relay reduce axis bug

* add tests for reduce bug
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 30, 2019
* fix relay reduce axis bug

* add tests for reduce bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants