-
Notifications
You must be signed in to change notification settings - Fork 666
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 scalar tensor.mean #8970
fix scalar tensor.mean #8970
Conversation
Flowingsun007
commented
Aug 19, 2022
- fix issue:MetaKD问题记录 models#378 (comment) 中记录的问题
- test case
…neflow into dev_fix_scalar_tensor.mean
for (int32_t i = 0; i < naxis; i++) { | ||
reduce_axis[i] = JUST(maybe_wrap_dim(axis[i], ndim)); | ||
axis_num[reduce_axis[i]]++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bug的原因在这一行,当x.mean(-1)
里的tensor x是scalar tensor时,则参数axis:{1}; ndim=0,reduce_axis为空vector,reduce_axis[i]会引发segment fault。
Code got formatted by CI. Please request CI again if you still want to have this PR merged. If the PR is from a forked repo, please download the patch files from the GitHub Actions web page and apply them locally. |
@@ -32,22 +32,21 @@ bool IsInplaceValid(const std::shared_ptr<Tensor>& x) { | |||
Maybe<std::vector<int32_t>> CheckAxis(const std::vector<int32_t>& axis, const int32_t& ndim) { | |||
const int32_t naxis = axis.size(); | |||
|
|||
int32_t reduce_ndim = naxis; | |||
if (naxis == 0 || ndim == 0) reduce_ndim = ndim; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里能解释下吗?没反应过来
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
哦,这里其实就是把两个判断合到一起了,如果naxis == 0,则reduce_ndim设置为ndim;如果ndim == 0(scalar tensor的情况),则reduce_ndim == 0(也==ndim)
const int32_t axis_i = JUST(maybe_wrap_dim(axis[i], ndim)); | ||
axis_num[axis_i]++; | ||
CHECK_OR_RETURN(axis_num[axis_i] < 2) << Error::RuntimeError() << "dim " << axis_i | ||
<< " appears multiple times in the list of dims"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里 else 分支似乎对返回值没有影响?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里应该是写错了(漏了),我再看一下
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已修改
View latest API docs preview at: https://staging.oneflow.info/docs/Oneflow-Inc/oneflow/pr/8970/ |
Speed stats:
|
@@ -35,23 +35,23 @@ bool IsScalarTensor(const std::shared_ptr<Tensor>& x) { | |||
|
|||
Maybe<std::vector<int32_t>> CheckAxis(const std::vector<int32_t>& axis, const int32_t& ndim) { | |||
const int32_t naxis = axis.size(); | |||
|
|||
int32_t reduce_ndim = naxis; | |||
if (naxis == 0 || ndim == 0) reduce_ndim = ndim; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
没有加大括号
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if后只有一句表达式,感觉不加也行😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我们代码的标准是要加的
CHECK_OR_RETURN_ERROR(axis_num[axis_i] < 2) << Error::RuntimeError() << "dim " << axis_i | ||
<< " appears multiple times in the list of dims"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这一段复用
oneflow/oneflow/core/common/wrap_dim_utils.h
Lines 45 to 58 in b9f51dd
static inline Maybe<std::bitset<dim_bitset_size>> dim_list_to_bitset( | |
const std::vector<int32_t>& dims, int64_t ndims) { | |
CHECK_LE_OR_RETURN(ndims, (int64_t)dim_bitset_size) | |
<< Error::RuntimeError() << "Only tensors with up to " << dim_bitset_size | |
<< " dims are supported"; | |
std::bitset<dim_bitset_size> seen; | |
for (int32_t i = 0; i < dims.size(); i++) { | |
size_t dim = JUST(maybe_wrap_dim(dims[i], ndims)); | |
CHECK_OR_RETURN(!seen[dim]) << Error::RuntimeError() << "The dim " << dim | |
<< " appears multiple times in the list of dims"; | |
seen[dim] = true; | |
} | |
return seen; | |
} |
axis_num[axis_i] < 2
也不太好
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已修改
View latest API docs preview at: https://staging.oneflow.info/docs/Oneflow-Inc/oneflow/pr/8970/ |
Speed stats:
|