From 4e9b6b454e1d057513ac477b2cd65f5925f91cc8 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Wed, 13 May 2020 01:29:03 +0000 Subject: [PATCH 1/4] Fix the issue of tf.divide's return value is not a tensor This PR fixes the issue of 39475 where tf.divide's return value is not a tensor in case x, y in divide(x, y) are both primitive python types. The reason was that tf.divide relies on implict `x / y`. However, if both x and y are not tensor, the return value will fall through python and will not be a tensor. This PR fixes the issue. This PR fixes 39475. Signed-off-by: Yong Tang --- tensorflow/python/ops/math_ops.py | 7 ++++++- tensorflow/python/ops/math_ops_test.py | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/tensorflow/python/ops/math_ops.py b/tensorflow/python/ops/math_ops.py index f062047cec2..b981af72e83 100644 --- a/tensorflow/python/ops/math_ops.py +++ b/tensorflow/python/ops/math_ops.py @@ -72,6 +72,7 @@ from __future__ import print_function import numpy as np import six +import sys from six.moves import builtins from six.moves import xrange # pylint: disable=redefined-builtin @@ -438,9 +439,13 @@ def divide(x, y, name=None): # override names. Use a dummy class to track the runtime division behavior return DivideDelegateWithName(x, name) / y else: + if not (isinstance(x, ops.Tensor) or isinstance(y, ops.Tensor)): + if sys.version_info.major < 3: + return _truediv_python2(x, y) + else: + return _truediv_python3(x, y) return x / y - @tf_export("math.multiply", "multiply") @dispatch.add_dispatch_support def multiply(x, y, name=None): diff --git a/tensorflow/python/ops/math_ops_test.py b/tensorflow/python/ops/math_ops_test.py index 2405eec9e49..dab0ea88ba8 100644 --- a/tensorflow/python/ops/math_ops_test.py +++ b/tensorflow/python/ops/math_ops_test.py @@ -495,6 +495,12 @@ class DivAndModTest(test_util.TensorFlowTestCase): # Consistent with desire to get numerator self.assertAllEqual(tf_result, expanded_nums) + def testWithPythonValue(self): + # Test case for GitHub issue 39475: + # https://github.com/tensorflow/tensorflow/issues/39475 + x = math_ops.divide(5, 2) + self.assertTrue(isinstance(x, ops.Tensor)) + @test_util.run_all_in_graph_and_eager_modes class DivNoNanTest(test_util.TensorFlowTestCase): From 410c66fa83537ea07e5158df915744931f461bfa Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Wed, 13 May 2020 16:22:40 +0000 Subject: [PATCH 2/4] Explain the reason to call _truediv_python3/_div_python2 explicitly (not through registered '/' Signed-off-by: Yong Tang --- tensorflow/python/ops/math_ops.py | 21 +++++++++++++++++++-- tensorflow/python/ops/math_ops_test.py | 3 +-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/tensorflow/python/ops/math_ops.py b/tensorflow/python/ops/math_ops.py index b981af72e83..2c141483eb1 100644 --- a/tensorflow/python/ops/math_ops.py +++ b/tensorflow/python/ops/math_ops.py @@ -439,13 +439,30 @@ def divide(x, y, name=None): # override names. Use a dummy class to track the runtime division behavior return DivideDelegateWithName(x, name) / y else: - if not (isinstance(x, ops.Tensor) or isinstance(y, ops.Tensor)): + # tf.math.divide will compute python style division x / y. As python 2 + # and python 3 have very much different semantics on `/` (__div__ vs. + # __truediv__), it would be natural to just use `x / y` as the operator + # '/' has already been registered for tensors, see + # _OverrideBinaryOperatorHelper for more details. + # However, in case both x and y are not tensors, the registered '/' + # _OverrideBinaryOperatorHelper will not take effect. In this case, + # python's default '/' operator will take effect which result in the return + # value of `tf.math.divide` as a non-Tensor. + # For that reason we excplicitly calls _truediv_python3/_div_python2 + # in case both x and y are not tensors. + # Since _truediv_python3/_div_python2 operates on tensors and will convert + # to tensor if needed. This avoid the situation of the following if not + # explicitly calling _truediv_python3/_div_python2: + # >>> tf.divide(5, 2) + # 2.5 <= should be instead. + if not (isinstance(x, ops.Tensor) or isinstance(y, ops.Tensor)): if sys.version_info.major < 3: - return _truediv_python2(x, y) + return _div_python2(x, y) else: return _truediv_python3(x, y) return x / y + @tf_export("math.multiply", "multiply") @dispatch.add_dispatch_support def multiply(x, y, name=None): diff --git a/tensorflow/python/ops/math_ops_test.py b/tensorflow/python/ops/math_ops_test.py index dab0ea88ba8..1debed531b6 100644 --- a/tensorflow/python/ops/math_ops_test.py +++ b/tensorflow/python/ops/math_ops_test.py @@ -498,10 +498,9 @@ class DivAndModTest(test_util.TensorFlowTestCase): def testWithPythonValue(self): # Test case for GitHub issue 39475: # https://github.com/tensorflow/tensorflow/issues/39475 - x = math_ops.divide(5, 2) + x = math_ops.divide(5, 2) self.assertTrue(isinstance(x, ops.Tensor)) - @test_util.run_all_in_graph_and_eager_modes class DivNoNanTest(test_util.TensorFlowTestCase): From 91914123c4239c13b15789e0147a6874e0abaf6c Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Wed, 13 May 2020 18:49:08 +0000 Subject: [PATCH 3/4] Simplify comment and use tensor_util.is_tensor and convert_to_tensor if both x and y are not tensor This is needed for `x / y` where both x and y are not tensors. As long as x is a tensor, `x / y` will work and will rely on _truediv_python3 to correctly figure out the right type for y. Signed-off-by: Yong Tang --- tensorflow/python/ops/math_ops.py | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/tensorflow/python/ops/math_ops.py b/tensorflow/python/ops/math_ops.py index 2c141483eb1..2d8285f709c 100644 --- a/tensorflow/python/ops/math_ops.py +++ b/tensorflow/python/ops/math_ops.py @@ -72,7 +72,6 @@ from __future__ import print_function import numpy as np import six -import sys from six.moves import builtins from six.moves import xrange # pylint: disable=redefined-builtin @@ -83,6 +82,7 @@ from tensorflow.python.framework import graph_util from tensorflow.python.framework import ops from tensorflow.python.framework import sparse_tensor from tensorflow.python.framework import tensor_shape +from tensorflow.python.framework import tensor_util from tensorflow.python.ops import array_ops from tensorflow.python.ops import gen_array_ops from tensorflow.python.ops import gen_data_flow_ops @@ -439,27 +439,9 @@ def divide(x, y, name=None): # override names. Use a dummy class to track the runtime division behavior return DivideDelegateWithName(x, name) / y else: - # tf.math.divide will compute python style division x / y. As python 2 - # and python 3 have very much different semantics on `/` (__div__ vs. - # __truediv__), it would be natural to just use `x / y` as the operator - # '/' has already been registered for tensors, see - # _OverrideBinaryOperatorHelper for more details. - # However, in case both x and y are not tensors, the registered '/' - # _OverrideBinaryOperatorHelper will not take effect. In this case, - # python's default '/' operator will take effect which result in the return - # value of `tf.math.divide` as a non-Tensor. - # For that reason we excplicitly calls _truediv_python3/_div_python2 - # in case both x and y are not tensors. - # Since _truediv_python3/_div_python2 operates on tensors and will convert - # to tensor if needed. This avoid the situation of the following if not - # explicitly calling _truediv_python3/_div_python2: - # >>> tf.divide(5, 2) - # 2.5 <= should be instead. - if not (isinstance(x, ops.Tensor) or isinstance(y, ops.Tensor)): - if sys.version_info.major < 3: - return _div_python2(x, y) - else: - return _truediv_python3(x, y) + # We do conversion here to make sure at least either x or y is a tensor. + if not (tensor_util.is_tensor(x) or tensor_util.is_tensor(y)): + x = ops.convert_to_tensor(x) return x / y From 39026d9e33040e9ff2a9d226543cfcac40e97010 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Wed, 13 May 2020 19:59:40 +0000 Subject: [PATCH 4/4] Remove the need to check if y is a tensor (always convert x if not) Signed-off-by: Yong Tang --- tensorflow/python/ops/math_ops.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tensorflow/python/ops/math_ops.py b/tensorflow/python/ops/math_ops.py index 2d8285f709c..03c10b37c95 100644 --- a/tensorflow/python/ops/math_ops.py +++ b/tensorflow/python/ops/math_ops.py @@ -439,8 +439,8 @@ def divide(x, y, name=None): # override names. Use a dummy class to track the runtime division behavior return DivideDelegateWithName(x, name) / y else: - # We do conversion here to make sure at least either x or y is a tensor. - if not (tensor_util.is_tensor(x) or tensor_util.is_tensor(y)): + # We do conversion here to make sure at least x is a tensor. + if not tensor_util.is_tensor(x): x = ops.convert_to_tensor(x) return x / y