Fix error message of multinomial op #27946

pangyoki · 2020-10-14T10:59:51Z

PR types

Bug fixes

PR changes

OPs

Describe

paddle.multinomial(x, num_samples=1, replacement=False, name=None) refer to PR #27219 .
Optimize the error message of some special situations.

QA test bugs that need to add error message

num_sample <= 0
Raise InvalidArgument Error and tell users that Number of samples should be > 0.
dimension of input x: dim_x <=0 or dim_x > 2
Raise InvalidArgument Error and tell users that Input probability distribution should be 1 or 2 dimension.

error behavior of CUDA Kernel is not consistent with CPU Kernel

value of element of x < 0
Because we need to calculate probabilities of distribution from x. It can't be less than 0.
In CUDA Kernel, Enforce x >= 0 and tell users that The input of multinomial distribution should be >= 0.
all of the elements of x are 0
Sum of elements of x should be >0.

By the way, fix multinomial and bernoulli python API's doc. Add the indentation.
Fix Categorical class's doc. Add the indentation, add attr name, change Variable to Tensor.
Add paddle.manual_seed in sample code.

… fix-multinomial

paddle-bot-old · 2020-10-14T10:59:56Z

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

zhiqiu

Please refer to https://github.com/PaddlePaddle/Paddle/wiki/Paddle-Error-Message-Writing-Specification to refine the error message.

zhiqiu · 2020-10-14T11:49:25Z

paddle/fluid/operators/multinomial_op.cc

    auto x_dim = ctx->GetInputDim("X");
    int64_t x_rank = x_dim.size();
+    PADDLE_ENFORCE_EQ(
+        x_rank > 0 && x_rank <= 2, true,


use PADDLE_ENFORCE_GT and PADDLE_ENFORCE_LE instead, do not combine two checks.

zhiqiu · 2020-10-14T11:51:03Z

paddle/fluid/operators/multinomial_op.cc


    int64_t num_samples = ctx->Attrs().Get<int>("num_samples");
+    PADDLE_ENFORCE_GT(num_samples, 0, platform::errors::OutOfRange(
+                                          "Number of samples should be > 0"));


Suggested change

"Number of samples should be > 0"));

"The number of samples should be > 0, but got %d.", num_samples ));

zhiqiu · 2020-10-14T11:52:10Z

paddle/fluid/operators/multinomial_op.cu

  int id = threadIdx.x + blockIdx.x * blockDim.x +
           blockIdx.y * gridDim.x * blockDim.x;
+  PADDLE_ENFORCE(in_data[id] >= 0.0,
+                 "The input of multinomial distribution should be >= 0");


Same above, print the actual data.

zhiqiu · 2020-10-14T11:54:52Z

paddle/fluid/operators/multinomial_op.cu

+  PADDLE_ENFORCE(in_data[id] >= 0.0,
+                 "The input of multinomial distribution should be >= 0");
+  PADDLE_ENFORCE(
+      !std::isinf(static_cast<double>(in_data[id])) &&


Please do not combine several logical expressions in one ENFORCE.

zhiqiu · 2020-10-16T02:35:11Z

paddle/fluid/operators/multinomial_op.cc

+                                     "1 or 2 dimension, but got %d",
+                                     x_rank));
+    PADDLE_ENFORCE_LE(x_rank, 2, platform::errors::PreconditionNotMet(
+                                     "Input probability distribution should be "


Suggested change

"Input probability distribution should be "

"The number of dimensions of the input probability distribution should be <= 2, but got %d."

Similar for the others.

zhiqiu · 2020-10-16T02:44:42Z

paddle/fluid/operators/multinomial_op.cu

+      in_data[id] >= 0.0,
+      "The input of multinomial distribution should be >= 0, but got %f",
+      in_data[id]);
+  PADDLE_ENFORCE(in_data[id] != INFINITY,


Is there any special reason that checking INF/NaN is added here? Otherwise, I think it is not really necessary. Because the property that a number is not NAN or INF should be satisfied almost everywhere, and if we check it everywhere, it may slow down the system.

have removed checking INF/NaN

zhiqiu · 2020-10-16T02:45:31Z

paddle/fluid/operators/multinomial_op.cu

+  PADDLE_ENFORCE(in_data[id] != NAN,
+                 "The input of multinomial distribution shoud not be NaN");
+  PADDLE_ENFORCE(sum_rows[blockIdx.y] > 0.0,
+                 "The sum of input should not be 0");


Do you mean >0 here?

Yes. It's > 0, and >0 has the same meaning with not be 0 here. Because <0 has been forbidden before.
I have change the description from not be 0 to >0.

zhiqiu · 2020-10-16T02:46:09Z

paddle/fluid/operators/multinomial_op.h

+      PADDLE_ENFORCE_EQ(
+          std::isinf(static_cast<double>(prob_value)), false,
          platform::errors::OutOfRange(
-              "The input of multinomial distribution should be >= 0"));
-      PADDLE_ENFORCE_EQ((std::isinf(static_cast<double>(prob_value)) ||
-                         std::isnan(static_cast<double>(prob_value))),
-                        false, platform::errors::OutOfRange(
-                                   "The input of multinomial distribution "
-                                   "shoud not be infinity or NaN"));
+              "The input of multinomial distribution shoud not be infinity"));
+      PADDLE_ENFORCE_EQ(
+          std::isnan(static_cast<double>(prob_value)), false,
+          platform::errors::OutOfRange(
+              "The input of multinomial distribution shoud not be NaN"));


same above.

zhiqiu · 2020-10-16T02:48:06Z

python/paddle/distribution.py

-          #  0.09053693, 0.30820143, 0.19095989]
+                x = paddle.rand([6])
+                print(x.numpy())
+                # [0.32564053, 0.99334985, 0.99034804,


Better add paddle.manual_seed(xx) here, otherwise, users cannot get the same random output as your sample code.
Same for all the other examples.

chenwhql · 2020-10-16T08:57:23Z

paddle/fluid/operators/multinomial_op.cu

  int id = threadIdx.x + blockIdx.x * blockDim.x +
           blockIdx.y * gridDim.x * blockDim.x;
+  PADDLE_ENFORCE(
+      in_data[id] >= 0.0,


建议报错信息统一加句点，PR里有的加了，有的没加

chenwhql

LGTM for PADDLE_ENFORCE

zhiqiu

LGTM

jzhang533

lgtm

pangyoki added 3 commits September 30, 2020 03:44

fix multinomial doc

9128de9

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

74ed1ee

… fix-multinomial

fix multinomial error message

2144fb6

little doc change

e6e54ea

zhiqiu reviewed Oct 14, 2020

View reviewed changes

fix Categorical class doc

1bb315b

pangyoki mentioned this pull request Oct 14, 2020

add categorical class #27695

Merged

pangyoki added 3 commits October 14, 2020 12:36

optimize format of error message

8f35154

fix CPU Kernel error message format

d5fe719

fix isinf and isnan error in WindowsOPENBLAS CI

f616d5b

zhiqiu reviewed Oct 16, 2020

View reviewed changes

pangyoki added 4 commits October 16, 2020 03:02

delete inf and nan

994713c

add manual_seed in sample code

01f331d

little error message change

8250e58

change error message to InvalidArgument

0b7f5cf

chenwhql reviewed Oct 16, 2020

View reviewed changes

add full point for error message and add manual_seed in CPU environment

9a3b6e4

chenwhql approved these changes Oct 16, 2020

View reviewed changes

zhiqiu approved these changes Oct 19, 2020

View reviewed changes

jzhang533 approved these changes Oct 19, 2020

View reviewed changes

zhiqiu merged commit 975bd88 into PaddlePaddle:develop Oct 19, 2020

This was referenced Oct 19, 2020

Cherry pick 27946 #28070

Closed

cherry pick 27946 Fix error message of multinomial op #28080

Merged

	"Number of samples should be > 0"));
	"The number of samples should be > 0, but got %d.", num_samples ));

	"Input probability distribution should be "
	"The number of dimensions of the input probability distribution should be <= 2, but got %d."

Fix error message of multinomial op #27946

Fix error message of multinomial op #27946

Uh oh!

Conversation

pangyoki commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR types

PR changes

Describe

QA test bugs that need to add error message

error behavior of CUDA Kernel is not consistent with CPU Kernel

Uh oh!

paddle-bot-old bot commented Oct 14, 2020

Uh oh!

zhiqiu left a comment

Choose a reason for hiding this comment

Uh oh!

zhiqiu Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenwhql left a comment

Choose a reason for hiding this comment

Uh oh!

zhiqiu left a comment

Choose a reason for hiding this comment

Uh oh!

jzhang533 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pangyoki commented Oct 14, 2020 •

edited

Loading

zhiqiu Oct 14, 2020 •

edited

Loading