[GitHub] [incubator-tvm] jwfromm commented on a change in pull request #6160: [ONNX]Mod operator, bug fix

[GitBox <gi...@apache.org>]

jwfromm commented on a change in pull request #6160:
URL: https://github.com/apache/incubator-tvm/pull/6160#discussion_r461686793



##########
File path: tests/python/frontend/onnx/test_forward.py
##########
@@ -2374,17 +2374,11 @@ def test_pooling():
                        auto_pad='SAME_UPPER')
 
 
-def verify_mod(x_shape, y_shape, fmod, dtype='float32'):
+def verify_mod(x_shape, y_shape, fmod, out_shape, dtype='float32'):
     x_np = np.random.uniform(size=x_shape).astype(dtype)
     y_np = np.random.uniform(size=y_shape).astype(dtype)

Review comment:
       These tests are only passing because `np.random.uniform` is always positive. If you use negative numbers, the behavior of `mod` changes based on the value of `fmod`. We should use a different random input generator than uniform anyway since values between 0 and 1 dont make sense for mod testing anyway.

##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -530,10 +530,7 @@ class Mod(OnnxOpConverter):
     @classmethod
     def _impl_v1(cls, inputs, attr, params):
         assert len(inputs) == 2, "Mod op take 2 inputs, {} given".format(len(inputs))
-        if attr['fmod'] == 1:
-            op_name = "floor_mod"
-        else:
-            op_name = "mod"
+        op_name = "mod"

Review comment:
       Some handling for `fmod=1` will need to be added here. From the onnx operator definition, `fmod=1` implies that the sign of the dividend should be kept in the output. When the dividend is negative, this differs from default relay behavior. We'll probably have to multiply the output by `relay.sign(inputs[0])` when `fmod=1` to make this work.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org