Skip to content

Problem with equality operator of expr.Mod? #540

Closed

Description

@wweic

I'm looking at unit test of ir builder, looks like the == operator does not work for Mod as expected:

    ib = tvm.ir_builder.create()
    n = tvm.var("n")
    A = ib.pointer("float32", name="A")
    with ib.for_range(0, n, name="i") as i:
        with ib.if_scope((i % 2) == 0):
            A[i] = A[i] + 1
        with ib.else_scope():
            A[0] = A[i] + 2

The generated IR is:

➜  ~ ipython
Python 2.7.10 (default, Jul 14 2015, 19:46:27)
Type "copyright", "credits" or "license" for more information.

IPython 5.1.0 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: import tvm
   ...:

In [2]:     ib = tvm.ir_builder.create()
   ...:     n = tvm.var("n")
   ...:     A = ib.pointer("float32", name="A")
   ...:     with ib.for_range(0, n, name="i") as i:
   ...:         with ib.if_scope((i % 2) == 0):
   ...:             A[i] = A[i] + 1
   ...:         with ib.else_scope():
   ...:             A[0] = A[i] + 2
   ...:
   ...:     body = ib.get()
   ...:

In [3]: body
Out[3]:
for (i, 0, n) {
  if (0) {
    A[i] = (A[i] + 1.000000f)
  } else {
    A[0] = (A[i] + 2.000000f)
  }
}

Note that the condition expression is constant 0 instead of an equality check expression. The __eq__ operator for Mod comes from tvm/_ffi/node.py

In [4]: i = ib.for_range(0, n, name="i")

In [5]: a = i._enter_value % 1

In [6]: a.__eq__.im_func
Out[6]: <function tvm._ffi.node.__eq__>

Which I assume what we want is tvm/expr.py?

I modify the unit test to use self.equal directly, and the code now makes sense to me:

In [7]: %paste
    ib = tvm.ir_builder.create()
    n = tvm.var("n")
    A = ib.pointer("float32", name="A")
    with ib.for_range(0, n, name="i") as i:
        with ib.if_scope((i % 2).equal(0)):
            A[i] = A[i] + 1
        with ib.else_scope():
            A[0] = A[i] + 2

    body = ib.get()
## -- End pasted text --

In [8]: body
Out[8]:
for (i, 0, n) {
  if (((i % 2) == 0)) {
    A[i] = (A[i] + 1.000000f)
  } else {
    A[0] = (A[i] + 2.000000f)
  }
}

Let me know if you think this is a problem. I'd be interested to help figure out why.

Activity

  1. tqchen commented on Oct 11, 2017

    @tqchen
    Member

    this is mainly because python do not allow simple overload of equal operator, we need to use tvm.equal instead

  2. wweic commented on Oct 11, 2017

    @wweic
    Author

    Thanks for looking. It should be fine to override equal operator in python. I think what's happening here is __eq__ in NodeBase override __eq__ in ExprOp. For instance I directly instantiate an object of ExprOp(so the object does not inherit from NodeBase), == works fine:

    In [50]: a = tvm.expr.ExprOp()
    
    In [51]: a == 1
    ---------------------------------------------------------------------------
    TypeError                                 Traceback (most recent call last)
    <ipython-input-51-8ff30fa1c86a> in <module>()
    ----> 1 a == 1
    
    /Library/Python/2.7/lib/python/site-packages/tvm-0.1.0-py2.7-macosx-10.10-intel.egg/tvm/expr.pyc in __eq__(self, other)
         88
         89     def __eq__(self, other):
    ---> 90         return self.equal(other)
         91
         92     def __ne__(self, other):
    

    Since I'm quite new to the codebase, not sure who is the consumer of the interface. If it's a piece of code that generate IR with the language, should be easy to just let that code stick to equal instead of ==.

  3. tqchen commented on Oct 11, 2017

    @tqchen
    Member

    I see, can you submit a patch to ExprOp to add this change? I agree that it is indeed helpful

  4. wweic commented on Oct 11, 2017

    @wweic
    Author

    I'll take a closer look and prepare a patch. thanks.

  5. wweic commented on Oct 12, 2017

    @wweic
    Author

    Turns out larger portion of tvm codebase assumes object identity semantics of == operator, instead of logical equality operator. After I force __eq__ to the one in ExprOp, a bunch of unit tests failed since they assumes object identity semantics(as defined in NodeBase.__eq__).

    ======================================================================
    ERROR: test_arith_detect_linear_equation.test_basic
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/python2.7/site-packages/nose/case.py", line 197, in runTest
        self.test(*self.arg)
      File "//tvm/tests/python/unittest/test_arith_detect_linear_equation.py", line 18, in test_basic
    
        assert m[0] == b  <-------  logical equality operator will fail here since assert will call bool() on the expression, which will cause failure due to ExprOp.__bool__().
    
      File "/tvm-0.1.0-py2.7-macosx-10.12-intel.egg/tvm/expr.py", line 103, in __nonzero__
        "use tvm.all / tvm.any instead")
    ValueError: Cannot use and / or / not operator to Expr, hint: use tvm.all / tvm.any instead
    

    I'll look if we can replace the usage of object identity with is operator, and use == for logical equality. This should make it clearer about the meaning of ==.

    For now I'll post a quick fix for the unit test first.

  6. tqchen commented on Oct 12, 2017

    @tqchen
    Member

    get it, thanks! We cannot use is operator either, because the underlying object are different(they share the same handle). What we could do is to introduce a same_as function similar to C++ end that does the same testing

  7. wweic commented on Oct 12, 2017

    @wweic
    Author

    That sounds good. I also prefer leaving the syntax sugar == for logical equality operator. Will follow up this later.

  8. wweic commented on Oct 13, 2017

    @wweic
    Author

    resolved in #550

  9. added a commit that references this issue on Jan 27, 2022
  10. added a commit that references this issue on Feb 2, 2022
  11. added a commit that references this issue on Feb 12, 2022
  12. added 2 commits that reference this issue on Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions