Conditions updated to cover better user scenarios

[ANSHUMAN87]

@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]

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]

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

[MarisaKirisame]

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]

@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]

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]

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]

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

[ANSHUMAN87]

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]

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

[tqchen]

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]

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]

ping @tqchen

[tqchen]

#5032

[tqchen]

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

[ANSHUMAN87]

@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!