Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix dtype not matching bug in log_prob and probs method of Distribution class #26767

Merged

Conversation

pangyoki
Copy link
Contributor

@pangyoki pangyoki commented Aug 28, 2020

PR types

Bug fixes

PR changes

APIs

Describe

In _to_tensor method of Distribution class (refer to PR #26355 and PR #26535). Even if we want to support both float32 and float64 dtype in Distribution classes, when parameters (low and high in Uniform, loc and scale in Normal) are numpy.ndarray and dtypes are float64, we can only set dtype to be float32 using assign op to get the correspoding variable. Becase assign op doesn't support float64 when input is numpy.ndarray.

Thus, when parameters are `numpy.ndarray` with `float64` dtype, 
we will transfer them into `VarType.FP32` variable.

In log_prob and probs methods in Distribution class, the input value of these methods is a tensor. In users' view, it's reasonable that the dtype of value and parameters are same.

When dtype of parameters are `float64` in `numpy.ndarray` and dtype of `value` is `VarType.FP64`, 
it will cause error because the dtype of parameters change to `VarType.FP32`. 

The following is an example code:

import numpy as np
import paddle
from paddle.distribution import Normal

paddle.disable_static()

value_np = np.array([0.8, 0.3], dtype='float64')
value_tensor = paddle.to_tensor(value_np)  # 'float64' Tensor

loc_np = np.array([1, 2]).astype('float64')  # will be converted to 'float32' Tensor automatically
scale_np = np.array([11, 22]).astype('float64')    # will be converted to 'float32' Tensor automatically
normal = Normal(loc_np, scale_np)

lp = normal.log_prob(value_tensor)  # error !!!

We are going to let assign op support float64, but it will lose precision because Attr don't support float64 in framework.proto (refer to #26797). That is, assign op can only support float32.

  • Thus, in this PR, we use cast operation to convert dtype after assign op if dtype is float64.
    If users define a Uniform distribution whose low and high are float64 numpy.ndarray, we firstly use assign op to get float32 variable. Then use cast to get float64 variable.

  • What's more, probs and log_prob methods have a variable input named values. If dtype of values is different with low in Uniform or loc in Normal, it will cause error.
    To solve this dtype conflict, we cast dtype of values to be the same as that of low or loc. (in _check_values_dtype_in_probs function)

  • In Doc discribtion, we add formula for entropy and kl-divergence methods. Formula for log_prob and probs have been given in doc of class, that is, the pdf (probability density function) of the distribution.

  • By the way, we rewrite unittest to make it more readable.

@paddle-bot-old
Copy link

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

@pangyoki pangyoki force-pushed the fix-_to_tensor-dtype-error-branch branch 6 times, most recently from 18029cd to 4da51a3 Compare September 2, 2020 19:32
raise TypeError(
"Type of input args must be float, list, numpy.ndarray or Tensor, but received type {}".
format(type(arg)))

arg_np = np.array(arg)
arg_dtype = arg_np.dtype
if str(arg_dtype) not in ['float32']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if str(arg_dtype) not in ['float32']:
if str(arg_dtype) != 'float32':

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, the positive conditions is better than the negative conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"data type of argument only support float32, your argument will be convert to float32."
)
# "assign" op doesn't support float64. if dtype is float64, float32 variable will be generated and transformed to float64 later using "cast".
if str(arg_dtype) not in ['float64']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if str(arg_dtype) not in ['float64']:
if str(arg_dtype) != 'float64':

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if isinstance(arg, float):
arg = np.zeros(1) + arg
arg = [arg]
elif not isinstance(arg, list) and not isinstance(arg, np.ndarray):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elif not isinstance(arg, list) and not isinstance(arg, np.ndarray):
elif not isinstance(arg, (list, np.ndarray)):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@pangyoki pangyoki force-pushed the fix-_to_tensor-dtype-error-branch branch from 30827ca to d3312c6 Compare September 3, 2020 16:29
@pangyoki pangyoki changed the title fix _to_tensor method of Distribution class fix dtype not matching bug in log_prob and probs method of Distribution class Sep 4, 2020
Copy link
Contributor

@zhiqiu zhiqiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jzhang533 jzhang533 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@zhiqiu zhiqiu merged commit a0c98e6 into PaddlePaddle:develop Sep 4, 2020
@pangyoki pangyoki mentioned this pull request Sep 7, 2020
pangyoki added a commit to pangyoki/Paddle that referenced this pull request Sep 7, 2020
…on class (PaddlePaddle#26767)

* fix _to_tensor method of Distribution class

* Add unittest

* let dtype be consistent with value in log_prob and probs

* fix format

* fix dtype problem and change unittest

* fix dtype of Numpy class in unittest

* add formula for entropy and kl

* change formula

* fix kl formula format

* fix kl formula format 2

* change gt to np in unittest

* optimize unittest format

* delete dumplicate

* delete dumplicate 2

* extract common function used to convert dtype value
zhiqiu pushed a commit that referenced this pull request Sep 7, 2020
* fix dtype not matching bug in log_prob and probs method of Distribution class (#26767)

* fix _to_tensor method of Distribution class

* Add unittest

* let dtype be consistent with value in log_prob and probs

* fix format

* fix dtype problem and change unittest

* fix dtype of Numpy class in unittest

* add formula for entropy and kl

* change formula

* fix kl formula format

* fix kl formula format 2

* change gt to np in unittest

* optimize unittest format

* delete dumplicate

* delete dumplicate 2

* extract common function used to convert dtype value

* cherry pick 27046
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants