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

Add support for overloading comparison operations in relay (#2910) #3168

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

u99127
Copy link
Contributor

@u99127 u99127 commented May 10, 2019

This commit adds support for overloaded comparison operators in the python binding for relay. I'm not sure how best to add testcases for this and couldn't find any obvious examples. A simple python script that uses all the comparison operators as in the issue #2910 report works just fine.

This is my first contribution to tvm, so all suggestions are welcome.

@jroesch
Copy link
Member

jroesch commented May 11, 2019

Looks good, could you add some tests cases to ensure it is working as intended?

@u99127
Copy link
Contributor Author

u99127 commented May 13, 2019

Thanks for the feedback . I've had a look and tried to add some testcases and there was something that puzzled me and I don't fully understand.

I would have expected a testcase like this to pass.

from tvm import relay
a = relay.Var("a")
b = relay.expr.const (1.0, dtype='float32')

c = (a == b)
d = relay.equal(a, b)
assert (c.astext() == d.astext())

But it fails with an assertion error because c evaluates to False statically.

while

c = (a < b)
d = relay.less (a, b)
assert (c.astext () == d.astext ())

passes.

Is this kind of discrepancy expected ?

@jroesch
Copy link
Member

jroesch commented May 14, 2019

We don't overload equality because it interacts with Python's collections is ways which are not desirable. You should just test the comparisons you added.

We can add an english method for checking equality if we want.

@tqchen tqchen added the status: need test case need test cases to cover the change label May 22, 2019
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

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

LGTM please add a few testcases

@tqchen
Copy link
Member

tqchen commented May 28, 2019

ping @u99127

This commit adds support for overloaded comparison operators
in the python binding for relay.

Add a testcase for this
@u99127
Copy link
Contributor Author

u99127 commented May 31, 2019

@tqchen - sorry I've been a bit busy recently. I've now pushed in my updates with the tests.

@tqchen tqchen merged commit 29b0b4c into apache:master Jun 5, 2019
@tqchen
Copy link
Member

tqchen commented Jun 5, 2019

thanks @u99127 @jroesch , this PR is now merged

@tqchen tqchen added status: accepted and removed status: need test case need test cases to cover the change labels Jun 5, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
@u99127 u99127 deleted the issue-2910 branch July 20, 2019 12:49
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