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.

Sign up for GitHub

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

Jump to bottom

fix the div 0 error of sequence_concat #49963

Merged
merged 8 commits into from
Feb 7, 2023
Merged

fix the div 0 error of sequence_concat #49963

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
004c1b7
fix the div 0 error of sequence_concat
Liyulingyue Jan 20, 2023
499b526
bug fix
Liyulingyue Jan 20, 2023
a180f53
Merge branch 'PaddlePaddle:develop' into div0_4
Liyulingyue Feb 1, 2023
a9a83aa
replace fluid with static
Liyulingyue Feb 1, 2023
23df7e2
fixed
Liyulingyue Feb 5, 2023
70f5049
fix error
Liyulingyue Feb 6, 2023
976f983
Merge branch 'PaddlePaddle:develop' into div0_4
Liyulingyue Feb 6, 2023
257c6b4
Update test_sequence_concat.py
Liyulingyue Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions python/paddle/fluid/layers/sequence_lod.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,10 @@ def sequence_concat(input, name=None):
['int64', 'float32', 'float64'],
'fluid.layers.sequence_concat',
)
if input_x.dim() == 1 and 0 in input_x.shape:
raise ValueError(
'the shape of element in input list should not be 0.'
)
Copy link

@iclementine iclementine Feb 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this change intended to prevent division by zero at SequenceConcatOp's InferShape?
image

If so, why only raise Exception when an input has shape [0]. In my opinion, when an input has a leading zero in its shape, like [0, 2, 3], an exception should be raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, I will try to solve it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this change intended to prevent division by zero at SequenceConcatOp's InferShape? image

If so, why only raise Exception when an input has shape [0]. In my opinion, when an input has a leading zero in its shape, like [0, 2, 3], an exception should be raised.

Hi, in the follow case, an exception have been writen.

If I simplely require that 'the input size must not be 0', some collisions may occur.

So, could you tell me where should I modify or what should I do?

import paddle
paddle.enable_static()
x = paddle.static.data(name='x', shape=[1,1], dtype='float32', lod_level=1)
y = paddle.static.data(name='y', shape=[1,0], dtype='float32', lod_level=1)
out = paddle.static.nn.sequence_concat(input=[x, y])

ValueError: Variable 'y' has been created before. The previous shape is (1, 0), the new shape is (0, 1). They are not matched.

Copy link
Member

Choose a reason for hiding this comment

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

If I simplely require that 'the input size must not be 0', some collisions may occur.

emmmm，就这个问题而言，因为是 InferShape 里出现了除第一个维度的需求，'the input size must not be 0' 的检查是不是不太合适？when an input has a leading zero in its shape 对此的检查就刚刚好

Hi, in the follow case, an exception have been writen.

ValueError: Variable 'y' has been created before. The previous shape is (1, 0), the new shape is (0, 1). They are not matched.

这个错误与本问题无关啊，只要你重新使用相同 name 定义了不同 shape 的 Variable 就会出现此错误，这里上文应该有一个定义过的 shape=(1, 0) 的 y，删掉或者将这个重命名即可（Jupyter 直接 Restart kernel）

So, could you tell me where should I modify or what should I do?

直接在 Python 端修改：

- if input_x.dim() == 1 and 0 in input_x.shape: # 仅仅要求 shape != [0]
+ if input_x.shape[0] == 0:                     # 要求 shape != [0, ...]

或者直接在 C++ 端增加一样的检查，比如这里 71 行后就很合适，个人觉得直接 C++ 端更好一些？

Paddle/paddle/fluid/operators/sequence_ops/sequence_concat_op.cc

Lines 71 to 86 in 3e5a6df

batch_size += x_dim[0];
if (feature_size == 0) {
feature_size = phi::product(x_dim) / x_dim[0];
} else {
PADDLE_ENFORCE_EQ(
feature_size,
phi::product(x_dim) / x_dim[0],
platform::errors::InvalidArgument(
"Each input of SequenceConcatOp inputs must have same feature "
"size, But "
"the feature size we received is %d, the feature size of 1st "
"input is %d",
feature_size,
phi::product(x_dim) / x_dim[0]));
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I simplely require that 'the input size must not be 0', some collisions may occur.

emmmm，就这个问题而言，因为是 InferShape 里出现了除第一个维度的需求，'the input size must not be 0' 的检查是不是不太合适？when an input has a leading zero in its shape 对此的检查就刚刚好

Hi, in the follow case, an exception have been writen.

ValueError: Variable 'y' has been created before. The previous shape is (1, 0), the new shape is (0, 1). They are not matched.

这个错误与本问题无关啊，只要你重新使用相同 name 定义了不同 shape 的 Variable 就会出现此错误，这里上文应该有一个定义过的 shape=(1, 0) 的 y，删掉或者将这个重命名即可（Jupyter 直接 Restart kernel）

So, could you tell me where should I modify or what should I do?

直接在 Python 端修改：

- if input_x.dim() == 1 and 0 in input_x.shape: # 仅仅要求 shape != [0]
+ if input_x.shape[0] == 0:                     # 要求 shape != [0, ...]

或者直接在 C++ 端增加一样的检查，比如这里 71 行后就很合适，个人觉得直接 C++ 端更好一些？

Paddle/paddle/fluid/operators/sequence_ops/sequence_concat_op.cc

Lines 71 to 86 in 3e5a6df

batch_size += x_dim[0];
if (feature_size == 0) {
feature_size = phi::product(x_dim) / x_dim[0];
} else {
PADDLE_ENFORCE_EQ(
feature_size,
phi::product(x_dim) / x_dim[0],
platform::errors::InvalidArgument(
"Each input of SequenceConcatOp inputs must have same feature "
"size, But "
"the feature size we received is %d, the feature size of 1st "
"input is %d",
feature_size,
phi::product(x_dim) / x_dim[0]));
}
}

更新一下报错信息~

ValueError: (InvalidArgument) Each input of SequenceConcatOp inputs must have same feature size, But the feature size we received is 1, the feature size of 1st input is 0
  [Hint: Expected feature_size == phi::product(x_dim) / x_dim[0], but received feature_size:1 != phi::product(x_dim) / x_dim[0]:0.] (at /paddle/paddle/fluid/operators/sequence_ops/sequence_concat_op.cc:84)
  [operator < sequence_concat > error]

Copy link

Choose a reason for hiding this comment

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

我也是觉得在 c++ 端添加 x_dim[0] 不能为 0 的检查会更好一些。

P.S. 由于现在 paddle 对 zero-sized tensor 支持不够好，所以目前添加这样的检查（无论是在 c++ 端还是在 Python 端）我觉得都 ok.

不过其实为了计算 feature_size 大可以用 product(x_shape[1: ]) 的方式来计算而不必用 x_numel / x_shape[0] 的方式计算. （P.S. paddle 的 lod tensor （多层不定长序列）的第 0 维表示递归折叠后的长度，而后面的可以称为 feature_shape，表示序列中的基础元素的形状）


out = helper.create_variable_for_type_inference(dtype=helper.input_dtype())
helper.append_op(
Expand Down
9 changes: 9 additions & 0 deletions python/paddle/fluid/tests/unittests/sequence/test_sequence_concat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,15 @@ def test_dtype():

self.assertRaises(TypeError, test_dtype)

def test_0_shape():
# dtype must be 'float32', 'float64', 'int64'
x4_data = paddle.static.data(name="x4", shape=[0], dtype='float32')
y4_data = paddle.static.data(name="y4", shape=[1], dtype='float32')
input_list = [x4_data, y4_data]
fluid.layers.sequence_concat(input=input_list)

self.assertRaises(ValueError, test_0_shape)


if __name__ == '__main__':
paddle.enable_static()
Expand Down