Conditions updated to cover better user scenarios by ANSHUMAN87 · Pull Request #4951 · apache/tvm

ANSHUMAN87 · 2020-02-27T06:56:26Z

@tqchen , @ZihengJiang, @jroesch, @merrymercy

Sorry if i miss any one who is relevant to this component.

tqchen

Thanks for the contribution, please add a regression test case to show what the new scenario did the PR cover

ANSHUMAN87 · 2020-02-29T16:15:07Z

Thanks for the contribution, please add a regression test case to show what the new scenario did the PR cover

@tqchen : I have uploaded test cases for the scenarios. Thanks!

tqchen · 2020-02-29T20:55:41Z

cc @slyubomirsky @MarisaKirisame @zhiics please help to take a look

MarisaKirisame · 2020-02-29T21:48:58Z

There is a neat trick to test for commutative bug.
In the test file, redefine alpha_equal to call type_analysis.alpha_equal twice, assert that the result is the same (whether argument flipped or not), then return the result.
Can you do this?

ANSHUMAN87 · 2020-03-01T09:54:33Z

@MarisaKirisame , @zhiics : Thanks for your efforts in reviewing code and your valuable comments. I have handled all your comments. Please correct me if i am mistaken anyplace. Thanks!

MarisaKirisame · 2020-03-01T12:17:03Z

can you, in tests/python/relay/test_pass_alpha_equal.py
def test_alpha_equal(x, y):
xy = alpha_equal(x, y)
yx = alpha_equal(y, x)
assert xy == yx
return xy

and use test_alpha_equal for those tests?

ANSHUMAN87 · 2020-03-01T15:23:58Z

can you, in tests/python/relay/test_pass_alpha_equal.py
def test_alpha_equal(x, y):
xy = alpha_equal(x, y)
yx = alpha_equal(y, x)
assert xy == yx
return xy
and use test_alpha_equal for those tests?

@MarisaKirisame : Thanks for such detailed description. I got your point from the last comment. But one of these scenarios is special. It was not possible to hit from python test cases. That is the reason i added in cpp test case. And i believe that the unit test cases should be in cpp, not in python, as the back-end implementations are in cpp. Python test cases should be used for functional tests. But that is my belief. I hope i didn't misunderstood your comment. Please help me clarify if i am mistaken. Thanks a lot!

MarisaKirisame · 2020-03-01T17:32:31Z

I agree that they should be in C++, but can you add such function hook there?

ANSHUMAN87 · 2020-03-02T05:46:53Z

I agree that they should be in C++, but can you add such function hook there?

@MarisaKirisame : Thanks! I have made changes as per your comments, please check!

zhiics

LGTM. One nitpick.

ANSHUMAN87 · 2020-03-02T07:25:02Z

@zhiics : Thanks for approving PR! I have made changes as per your feedback. Please check. Thanks!

tqchen · 2020-03-09T20:00:30Z

This PR is among one of the PRs affected by the github squash commit bug. We take every contribution serious in the TVM community. The community has decided to use revert/redo approach to amend the contributions as per #5015

@ANSHUMAN87 Please let us know if you would like us to revert the PR and resend the contribution. Thank you

ANSHUMAN87 · 2020-03-10T04:30:56Z

This PR is among one of the PRs affected by the github squash commit bug. We take every contribution serious in the TVM community. The community has decided to use revert/redo approach to amend the contributions as per #5015
@ANSHUMAN87 Please let us know if you would like us to revert the PR and resend the contribution. Thank you

@tqchen : Thanks a lot! Please revert the merged one. I will raise again.

masahi · 2020-03-10T20:50:54Z

ping @tqchen

This reverts commit fe74b37.

tqchen · 2020-03-10T20:59:26Z

#5032

…5032) This reverts commit fe74b37.

tqchen · 2020-03-11T15:56:55Z

@ANSHUMAN87 the revert PR has been merged, please send another PR to add the patch back :)

ANSHUMAN87 · 2020-03-11T16:39:16Z

@ANSHUMAN87 the revert PR has been merged, please send another PR to add the patch back :)

@tqchen: Thanks a lot! I have raised new PR now(#5043). Please check and approve. Thanks!

* Conditions updated to cover better user scenarios * [1] New test case added * [2] New test case added * [3] Proper variable name used * [4] Review Comments handled * [5] Review comments handled * [6] Review comments handled

…" (apache#5032) This reverts commit fe74b37.

* Conditions updated to cover better user scenarios * [1] New test case added * [2] New test case added * [3] Proper variable name used * [4] Review Comments handled * [5] Review comments handled * [6] Review comments handled

…" (apache#5032) This reverts commit fe74b37.

Conditions updated to cover better user scenarios
a01ce2c

tqchen approved these changes Feb 27, 2020
View reviewed changes

tqchen requested changes Feb 27, 2020
View reviewed changes

tqchen added status: need review status: need test case need test cases to cover the change labels Feb 27, 2020

ANSHUMAN TRIPATHY added 2 commits February 29, 2020 21:16

[1] New test case added
f93a688

[2] New test case added
af9c68c

[3] Proper variable name used
742bec5

zhiics reviewed Feb 29, 2020
View reviewed changes

Comment threadtests/cpp/relay_pass_alpha_equal.cc

[4] Review Comments handled
d0c53b0

[5] Review comments handled
e5522cf

zhiics approved these changes Mar 2, 2020
View reviewed changes

Comment threadtests/python/relay/test_pass_alpha_equal.py Outdated

[6] Review comments handled
4b7909a

MarisaKirisame approved these changes Mar 2, 2020
View reviewed changes

tqchen approved these changes Mar 5, 2020
View reviewed changes

tqchen merged commit fe74b37 into apache:masterMar 5, 2020

tqchen added status: accepted and removed status: need review status: need test case need test cases to cover the change labels Mar 5, 2020

tqchen mentioned this pull request Mar 9, 2020
[DEV] Amend Github Attribution Bug #5015
Closed
5 tasks

tqchen added a commit that referenced this pull request Mar 10, 2020
Revert "Conditions updated to cover better user scenarios (#4951)"
07bac24
This reverts commit fe74b37.

tqchen mentioned this pull request Mar 10, 2020
Revert "Conditions updated to cover better user scenarios" #5032
Merged

tqchen added a commit that referenced this pull request Mar 11, 2020
Revert "Conditions updated to cover better user scenarios (#4951)" (#…
99af7c5
…5032) This reverts commit fe74b37.

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
Revert "Conditions updated to cover better user scenarios (apache#4951)…
7f7b0d0
…" (apache#5032) This reverts commit fe74b37.

zhiics pushed a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
Revert "Conditions updated to cover better user scenarios (apache#4951)…
535095a
…" (apache#5032) This reverts commit fe74b37.

ZihengJiang mentioned this pull request Sep 17, 2020
TVM v0.7 Release Note Candidate #6486
Closed

Uh oh!

Conversation

ANSHUMAN87 commented Feb 27, 2020

Uh oh!

tqchen left a comment

Choose a reason for hiding this comment

Uh oh!

ANSHUMAN87 commented Feb 29, 2020

Uh oh!

tqchen commented Feb 29, 2020

Uh oh!

Uh oh!

MarisaKirisame commented Feb 29, 2020

Uh oh!

ANSHUMAN87 commented Mar 1, 2020

Uh oh!

MarisaKirisame commented Mar 1, 2020

Uh oh!

ANSHUMAN87 commented Mar 1, 2020

Uh oh!

MarisaKirisame commented Mar 1, 2020

Uh oh!

ANSHUMAN87 commented Mar 2, 2020

Uh oh!

zhiics left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ANSHUMAN87 commented Mar 2, 2020

Uh oh!

tqchen commented Mar 9, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ANSHUMAN87 commented Mar 10, 2020

Uh oh!

masahi commented Mar 10, 2020

Uh oh!

tqchen commented Mar 10, 2020

Uh oh!

tqchen commented Mar 11, 2020

Uh oh!

ANSHUMAN87 commented Mar 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tqchen commented Mar 9, 2020•
edited
Loading