Fix the accuracy problem of allclose op when using float64 data type #27891

huangxu96 · 2020-10-13T08:12:23Z

PR types

Bug fixes

PR changes

OPs

Describe

This PR fixed a bug in allclose_op, which cannot get the expected output when fp64 as input in some cases.

BUG reproduce process:

paddle.disable_static()

np_x = np.array([10.1]).astype("float64")

np_y = np.array([10]).astype("float64")

x = paddle.to_tensor (np_x)

y = paddle.to_tensor (np_y)

result = paddle.allclose(x=x, y=y, rtol=0.01, atol=0, equal_nan=False, name="ignore_nan")

result = result.numpy()

print(result)

This result is expected to be True but it returns false.

Problem reason

Floating point number cannot be determined equal directly, since floating point number cannot have a precise experssion in
Computer. For example, 0.1 might be 0.09999 or 0.100001 in computer. So when we want to determine two "0.1" if they are equaled, the compute might executes if 0.09999 equal to 0.100001. This is how the false result comes.

Solving approach:

Add an extremely small value (1e-15) when determine if two double varibles are equaled.

Change the date type of rtol and atol from float32 to float64, since the accuracy of rtol and atol will also impact the final result.

… allclose_op

paddle-bot-old · 2020-10-13T08:12:27Z

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

wzzju · 2020-10-13T08:25:50Z

paddle/fluid/operators/allclose_op.cu

+  T operator()(const framework::Tensor& tensor) const {
+    const T* data = tensor.data<T>();
+    T value;
+    cudaMemcpy(&value, data, sizeof(T), cudaMemcpyDeviceToHost);


Prefer to use memory::Copy(platform::CPUPlace(), &value, gpu_place, data, sizeof(T), dev_ctx.stream());

wzzju · 2020-10-13T08:32:45Z

paddle/fluid/operators/allclose_op.cc

+    auto* in_a = in.data<T>();
+    auto* in_b = other.data<T>();


Use better var name instead.

wzzju · 2020-10-13T08:34:04Z

paddle/fluid/operators/allclose_op.cc

+      } else {
+        T left = (a > b ? a - b : b - a);
+        T right = atol + (b > 0 ? rtol * b : (-rtol) * b);
+        T dif = (left > right ? left - right : right - left);


Use diff instead.

wzzju · 2020-10-13T08:37:17Z

paddle/fluid/operators/allclose_op.cc

+  }
+};
+
+template struct AllcloseFunctor<platform::CPUDeviceContext, double>;


Not needed here.

wzzju · 2020-10-13T08:41:54Z

paddle/fluid/operators/allclose_op.cu

+  } else {
+    T left = (a > b ? a - b : b - a);
+    T right = atol + (b > 0 ? rtol * b : (-rtol) * b);
+    T dif = (left > right ? left - right : right - left);


Use diff instead.

wzzju · 2020-10-13T08:43:58Z

paddle/fluid/operators/allclose_op.cu

+  atomicAnd(reinterpret_cast<int*>(&val_), static_cast<int>(val));
+  __syncthreads();
+  if (tid == 0) {
+    *out_data = static_cast<bool>(val_);


Here static_cast is not needed.

Already used parallel reduction here.

wzzju · 2020-10-13T08:48:52Z

paddle/fluid/operators/allclose_op.cu

+};
+
+template <typename T>
+__global__ void AllcloseCUDAKernel(const T* in_a, const T* in_b,


The performance here is too poor.

Already used parallel reduction here.

wzzju · 2020-10-13T09:01:57Z

paddle/fluid/operators/allclose_op.cu

+    int grid = 1;
+    int block = in_dims;


Think over and try to instead these ridiculous codes here.

wzzju · 2020-10-13T09:06:09Z

python/paddle/fluid/tests/unittests/test_allclose_op.py

@@ -22,19 +22,20 @@ class TestAllcloseOp(OpTest):
    def set_args(self):


Add the test case below.

Superjomn

考虑到 allclose 在推理里几乎用不到

这次升级这里暂不考虑兼容

…ut, and added an unittest for it.

wzzju · 2020-10-16T04:55:30Z

python/paddle/fluid/tests/unittests/test_allclose_op.py

+class TestAllcloseOpFloat64(TestAllcloseOp):
+    def set_args(self):
+        self.input = np.array([10.1]).astype("float64")
+        self.other = np.array([10]).astype("float64")
+        self.rtol = np.array([0.01]).astype("float64")
+        self.atol = np.array([0]).astype("float64")
+        self.equal_nan = False


Add the same unit test for float32.

wzzju · 2020-10-16T04:55:49Z

paddle/fluid/operators/allclose_op.cu

+  }
+};
+
+template struct AllcloseFunctor<platform::CUDADeviceContext, double>;


Remove this line.

… allclose_op

wzzju

LGTM.

Superjomn

LGTM

* Still has bugs. * Fixed allclose_op bug, which cannot deal with some cases of fp64 inputs. * improved CUDA kernel performance. * Changed CUDA code. * Fixed a bug in cuda kernel which cannot deal with large dimension input, and added an unittest for it. * Add a test case for float32 input.

* Fixed allclose_op bug, which cannot deal with some cases of fp64 inputs. * improved CUDA kernel performance. * Fixed a bug in cuda kernel which cannot deal with large dimension input, and added an unit test for it. * Add a test case for float32 input.

huangxu96 added 3 commits September 30, 2020 10:33

Still has bugs.

20dc8c8

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

cc3e383

… allclose_op

Fixed allclose_op bug, which cannot deal with some cases of fp64 inputs.

ec784fc

wzzju requested changes Oct 13, 2020

View reviewed changes

huangxu96 added 2 commits October 13, 2020 12:03

improved CUDA kernel performance.

98507ae

Changed CUDA code.

53ee2cb

huangxu96 force-pushed the allclose_op branch from 3570913 to 53ee2cb Compare October 13, 2020 14:10

Superjomn previously approved these changes Oct 14, 2020

View reviewed changes

huangxu96 dismissed Superjomn’s stale review via afdd373 October 14, 2020 12:21

Fixed a bug in cuda kernel which cannot deal with large dimension inp…

fbc7d20

…ut, and added an unittest for it.

huangxu96 force-pushed the allclose_op branch from afdd373 to fbc7d20 Compare October 14, 2020 13:26

wzzju reviewed Oct 16, 2020

View reviewed changes

huangxu96 added 2 commits October 16, 2020 08:49

Add a test case for float32 input.

e4535af

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

be7ada2

… allclose_op

wzzju approved these changes Oct 19, 2020

View reviewed changes

Superjomn approved these changes Oct 19, 2020

View reviewed changes

wzzju merged commit d466893 into PaddlePaddle:develop Oct 19, 2020

huangxu96 changed the title ~~Allclose op~~ Allclose op bug fixed Oct 19, 2020

wzzju changed the title ~~Allclose op bug fixed~~ Fix the accuracy problem of allclose op when using float64 data type. Oct 19, 2020

huangxu96 changed the title ~~Fix the accuracy problem of allclose op when using float64 data type.~~ Fixed a bug of allclose op that cannot get the expected output when fp64 data as input in some cases Oct 19, 2020

huangxu96 changed the title ~~Fixed a bug of allclose op that cannot get the expected output when fp64 data as input in some cases~~ Fix the accuracy problem of allclose op when using float64 data type Oct 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the accuracy problem of allclose op when using float64 data type #27891

Fix the accuracy problem of allclose op when using float64 data type #27891

huangxu96 commented Oct 13, 2020 •

edited

Loading

paddle-bot-old bot commented Oct 13, 2020

wzzju Oct 13, 2020

huangxu96 Oct 13, 2020

wzzju Oct 13, 2020

huangxu96 Oct 14, 2020

wzzju Oct 13, 2020

huangxu96 Oct 13, 2020

wzzju Oct 13, 2020

huangxu96 Oct 13, 2020

wzzju Oct 13, 2020

huangxu96 Oct 13, 2020

wzzju Oct 13, 2020

huangxu96 Oct 13, 2020

wzzju Oct 13, 2020

huangxu96 Oct 13, 2020

wzzju Oct 13, 2020

huangxu96 Oct 13, 2020

wzzju Oct 13, 2020

huangxu96 Oct 13, 2020

Superjomn left a comment

wzzju Oct 16, 2020

huangxu96 Oct 16, 2020

wzzju Oct 16, 2020

huangxu96 Oct 16, 2020

wzzju left a comment

Superjomn left a comment

		@@ -22,19 +22,20 @@ class TestAllcloseOp(OpTest):
		def set_args(self):

Fix the accuracy problem of allclose op when using float64 data type #27891

Fix the accuracy problem of allclose op when using float64 data type #27891

Conversation

huangxu96 commented Oct 13, 2020 • edited Loading

PR types

PR changes

Describe

paddle-bot-old bot commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Superjomn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzzju left a comment

Choose a reason for hiding this comment

Superjomn left a comment

Choose a reason for hiding this comment

huangxu96 commented Oct 13, 2020 •

edited

Loading