[ONNX]Mod operator, bug fix #6160
Conversation
| @@ -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) | |||
There was a problem hiding this 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.
python/tvm/relay/frontend/onnx.py
Outdated
| op_name = "floor_mod" | ||
| else: | ||
| op_name = "mod" | ||
| op_name = "mod" |
There was a problem hiding this 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.
siju-samuel
force-pushed
the
onnx_fmod_flaky_test
branch
from
cf38f8a to
6751c29
Compare
|
Thanks for the changes, this looks good to me now. |
|
@masahi, this PR made me realize that |
|
I don't know if this is intentional or not, but since we cannot change Relay definition easily, having workaround in the frontend side seems a better alternative. |
| @@ -530,10 +530,11 @@ 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: | |||
| if attr['fmod'] == 0: | |||
There was a problem hiding this comment.
Can we add a comment here to avoid confusion?
|
Thanks @siju-samuel @jwfromm |
* Onnx mod, bug fix * Added comment for the mod/floor_mod behaviour difference between numpy & relay
* Onnx mod, bug fix * Added comment for the mod/floor_mod behaviour difference between numpy & relay
* Onnx mod, bug fix * Added comment for the mod/floor_mod behaviour difference between numpy & relay
* Onnx mod, bug fix * Added comment for the mod/floor_mod behaviour difference between numpy & relay
* Onnx mod, bug fix * Added comment for the mod/floor_mod behaviour difference between numpy & relay
* Onnx mod, bug fix * Added comment for the mod/floor_mod behaviour difference between numpy & relay
#6106
Reference Onnx mod
@jwfromm Please help to review this PR. TIA