You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/08/07 09:58:22 UTC
[GitHub] [incubator-tvm] shiwenloong opened a new pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
shiwenloong opened a new pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232
Unbiased variance uses `N-1` as the divisor in the calculation, where N represents the number of elements. `torch.std` and `torch.var` are unbiased by default in pytorch and these unbiased ops can't be converted.
This PR adds unbiased variance op and corresponding support in pytorch frontend.
@masahi @junrushao1994 Please help to review this PR. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] shiwenloong commented on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
shiwenloong commented on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-670835116
> Do we need to introduce new Relay ops? Can we just add `unbiased` argument to existing ops like PyTorch?
Existing variance op is a reduce op, and all reduce ops take `axis`, `keepdims` and `exclude` as attributes. I think adding `unbiased` argument to variance op will change the consistency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] shiwenloong commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
shiwenloong commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467018665
##########
File path: python/tvm/relay/frontend/pytorch.py
##########
@@ -1263,27 +1263,32 @@ def _impl(inputs, input_types):
unbiased = bool(inputs[2])
if unbiased:
- msg = "Currently only supports standard-deviation calculated via the biased "\
- "estimator. PyTorch's Bessel's correction is not supported."
- raise NotImplementedError(msg)
+ std_op = _op.reduce.unbiased_std
+ else:
+ std_op = _op.reduce.std
Review comment:
This change will make the code cleaner. It has been applied. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] leandron commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r466997354
##########
File path: python/tvm/relay/op/reduce.py
##########
@@ -376,6 +408,39 @@ def std(data, axis=None, keepdims=False, exclude=False):
return sqrt(_make._variance(data, m, axis, keepdims, exclude))
+def unbiased_std(data, axis=None, keepdims=False, exclude=False):
+ """Computes the unbiased standard deviation of data over given axes.
+
+ Parameters
+ ----------
+ data : relay.Expr
+ The input data
+
+ axis : None or int or tuple of int
+ Axis or axes along which a standard deviation operation is performed.
+ The default, axis=None, will compute the standard deviation of all elements in the
+ input array. If axis is negative it counts from the last to the first axis.
+
+ keepdims : bool
+ If this is set to True, the axes which are reduced are left in the result as dimensions
+ with size one.
+ With this option, the result will broadcast correctly against the input array.
+
+ exclude : bool
+ If `exclude` is true, reduction will be performed on the axes that are
+ NOT in axis instead.
+
+ Returns
+ -------
+ result : relay.Expr
+ The computed result.
+ """
+ axis = [axis] if isinstance(axis, int) else axis
+ m = mean(data, axis, True, exclude)
+
+ return sqrt(_make._unbiased_variance(data, m, axis, keepdims, exclude))
Review comment:
I'm not very familiar with the specifics here, but, could this be replaced by the example below, to avoid repetition?
```
return sqrt(unbiased_variance(data, axis, keepdims, exclude))
```
##########
File path: python/tvm/relay/frontend/pytorch.py
##########
@@ -1263,27 +1263,32 @@ def _impl(inputs, input_types):
unbiased = bool(inputs[2])
if unbiased:
- msg = "Currently only supports standard-deviation calculated via the biased "\
- "estimator. PyTorch's Bessel's correction is not supported."
- raise NotImplementedError(msg)
+ std_op = _op.reduce.unbiased_std
+ else:
+ std_op = _op.reduce.std
Review comment:
minor suggestion: you could use the same as you did below with `axis`, to make this statement shorter.
```
std_op = _op.reduce.unbiased_std if unbiased else _op.reduce.std
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467647366
##########
File path: src/relay/op/tensor/reduce.cc
##########
@@ -193,15 +194,19 @@ Array<te::Tensor> ReduceCompute(const Attrs& attrs, const Array<te::Tensor>& inp
/*!
* \brief ReduceShapeImpl get the outshape for the reduction operator
* \param in_shape Shape of input data.
- * \param param ReduceAttrs details.
+ * \param inaxis The input axis vector.
+ * \param keepdims If this is set to true, the axes which are reduced are
+ * left in the result as dimensions with size one. This enables the result
+ * to broadcast correctly against the input array.
+ * \param exclude Whether 'axis' input given is the excluded axis.
* \param reporter The reporter to report solution to.
* \return oshape Output shape inferred.
*/
inline std::vector<IndexExpr> ReduceShapeImpl(const std::vector<IndexExpr>& in_shape,
- const ReduceAttrs* param,
- const TypeReporter& reporter) {
Review comment:
Let's make `param` here a template, so that `ReduceShapeImpl` can take both `ReduceAttrs` and `VarianceAttrs` (since they have the same attributes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] shiwenloong commented on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
shiwenloong commented on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-671093947
> I mean, the new `VarianceAttrs` should also have all attributes of `ReduceAttrs`. The underlying implementation of variance op can continue to be the reduce op.
>
>
>
> Isn't updating `VarianceCompute` (like you did) and `MakeVariance` (to add `unbiased` param with default False) with corresponding changes in `reduce.py` enough? I don't see why we'd need to register a new shape func to variance op. I don't think it would break anything either.
>
>
>
> Sorry I didn't look into the details, so I may be missing something.
I try the way of of adding `VarianceAttrs` in current implementation. There are fewer problems than I thought. But the inconsistency of `reduce.py` still exists because `std` and `variance` take one more parameter than all other functions in the file. I am not sure if this is a noteworthy issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467661956
##########
File path: src/relay/op/tensor/reduce.cc
##########
@@ -266,7 +269,7 @@ bool ArgReduceRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
CHECK(param != nullptr);
// assign output type and shape
- auto oshape = ReduceShapeImpl(in_shape, param, reporter);
+ auto oshape = ReduceShapeImpl<ReduceAttrs>(in_shape, param, reporter);
Review comment:
no need to add explicit template annotation, it should be inferred
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] shiwenloong commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
shiwenloong commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467022417
##########
File path: python/tvm/relay/op/reduce.py
##########
@@ -376,6 +408,39 @@ def std(data, axis=None, keepdims=False, exclude=False):
return sqrt(_make._variance(data, m, axis, keepdims, exclude))
+def unbiased_std(data, axis=None, keepdims=False, exclude=False):
+ """Computes the unbiased standard deviation of data over given axes.
+
+ Parameters
+ ----------
+ data : relay.Expr
+ The input data
+
+ axis : None or int or tuple of int
+ Axis or axes along which a standard deviation operation is performed.
+ The default, axis=None, will compute the standard deviation of all elements in the
+ input array. If axis is negative it counts from the last to the first axis.
+
+ keepdims : bool
+ If this is set to True, the axes which are reduced are left in the result as dimensions
+ with size one.
+ With this option, the result will broadcast correctly against the input array.
+
+ exclude : bool
+ If `exclude` is true, reduction will be performed on the axes that are
+ NOT in axis instead.
+
+ Returns
+ -------
+ result : relay.Expr
+ The computed result.
+ """
+ axis = [axis] if isinstance(axis, int) else axis
+ m = mean(data, axis, True, exclude)
+
+ return sqrt(_make._unbiased_variance(data, m, axis, keepdims, exclude))
Review comment:
This replacement is of no problem. But the current implementation of `unbiased_variance` and `unbiased_std` follows how `variance` and `std` were implemented. I think taking the same implementation style will be better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-671178412
Thanks @shiwenloong @leandron
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467646978
##########
File path: include/tvm/relay/attrs/reduce.h
##########
@@ -60,6 +60,37 @@ struct ReduceAttrs : public tvm::AttrsNode<ReduceAttrs> {
"Whether to perform reduction on axis that are NOT in axis instead.");
}
};
+
+struct VarianceAttrs : public tvm::AttrsNode<VarianceAttrs> {
Review comment:
I wonder if we can inherit from `ReduceAttrs`. Not sure if the attribute system works in that case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-671120513
> But the inconsistency of `reduce.py` still exists because `std` and `variance` take one more parameter than all other functions in the file. I am not sure if this is a noteworthy issue.
They look fine to me. I don't think a small API change like this is a problem at all. Being in the same file doesn't mean they have to have the same signature.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] shiwenloong edited a comment on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
shiwenloong edited a comment on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-670835116
> Do we need to introduce new Relay ops? Can we just add `unbiased` argument to existing ops like PyTorch?
The existing variance op is a reduce op, and all reduce ops take `axis`, `keepdims` and `exclude` as attributes. I think adding `unbiased` argument to variance op will change the consistency. It will also bring more work to isolate the variance op from reduce ops.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-670975972
I mean, the new `VarianceAttrs` should also have all attributes of `ReduceAttrs`. The underlying implementation of variance op can continue to be the reduce op.
Isn't updating `VarianceCompute` (like you did) and `MakeVariance` (to add `unbiased` param with default False) enough? I don't see why we'd need to register a new shape func to variance op. I don't think it would break anything either.
Sorry I didn't look into the details, so I may be missing something.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi edited a comment on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-670975972
I mean, the new `VarianceAttrs` should also have all attributes of `ReduceAttrs`. The underlying implementation of variance op can continue to be the reduce op.
Isn't updating `VarianceCompute` (like you did) and `MakeVariance` (to add `unbiased` param with default False) with corresponding changes in `reduce.py` enough? I don't see why we'd need to register a new shape func to variance op. I don't think it would break anything either.
Sorry I didn't look into the details, so I may be missing something.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] shiwenloong commented on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
shiwenloong commented on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-670922682
> > I think adding `unbiased` argument to variance op will change the consistency. It will also bring more work to isolate the variance op from reduce ops.
>
> I'm not completely sure what you mean by above, but how about adding `VarianceAttrs` with default `unbiased=false`? I think that should fix the coupling between variance and reduce op you are referring to?
>
> Anyway, adding two new ops just for the sake of adding `unbiased` support sounds a bit overkill to me.
If we follow this modification method, we need to add more than just `VarianceAttrs`. Many codes and tests related to the variance operator need to be changed. For example, we should also add `variance_shape_func` in `python/tvm/relay/op/_reduce.py`. `std` and `variance` should be isolated from `python/tvm/relay/op/reduce.py` because their parameters are different from other reduce ops.
In my opinion, although adding two unbiased operators seems a bit redundant, it has little impact on the code structure and is of a lower probability of causing bugs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467647626
##########
File path: src/relay/op/tensor/reduce.cc
##########
@@ -542,51 +547,61 @@ bool VarianceRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
std::vector<IndexExpr> mean_shape(mean->shape.begin(), mean->shape.end());
CHECK_EQ(in_shape.size(), mean_shape.size());
- const ReduceAttrs* param = attrs.as<ReduceAttrs>();
+ const VarianceAttrs* param = attrs.as<VarianceAttrs>();
CHECK(param != nullptr);
// assign output type and shape
- auto oshape = ReduceShapeImpl(in_shape, param, reporter);
+ auto oshape = ReduceShapeImpl(in_shape, param->axis, param->keepdims, param->exclude, reporter);
reporter->Assign(types[2], TensorType(oshape, data->dtype));
return true;
}
Array<te::Tensor> VarianceCompute(const Attrs& attrs, const Array<te::Tensor>& inputs,
const Type& out_type) {
IndexExpr count = tir::make_const(inputs[0]->dtype, 1);
- const ReduceAttrs* param = attrs.as<ReduceAttrs>();
+ const VarianceAttrs* param = attrs.as<VarianceAttrs>();
CHECK(param != nullptr);
auto axes = param->axis;
+ bool unbiased = param->unbiased;
auto data = inputs[0];
auto mean = inputs[1];
for (int64_t i : GetReduceAxes(data->shape.size(), param->axis, param->exclude)) {
count *= data->shape[i];
}
+ if (unbiased) {
+ count -= 1;
+ }
std::vector<Integer> expand_shape;
auto sq_diff = topi::power(topi::subtract(data, mean), 2);
- auto var = topi::divide(ReduceCompute(attrs, {sq_diff}, out_type, topi::sum)[0], count);
Review comment:
If `VarianceAttrs` could inherit from `ReduceAttrs`, we don't have to introduce this change. But again, I don't know if that inheritance of Attrs works...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467660320
##########
File path: include/tvm/relay/attrs/reduce.h
##########
@@ -60,6 +60,37 @@ struct ReduceAttrs : public tvm::AttrsNode<ReduceAttrs> {
"Whether to perform reduction on axis that are NOT in axis instead.");
}
};
+
+struct VarianceAttrs : public tvm::AttrsNode<VarianceAttrs> {
Review comment:
ok, thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-670787826
Do we need to introduce new Relay ops? Can we just add `unbiased` argument to existing ops like PyTorch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] leandron commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467041812
##########
File path: python/tvm/relay/op/reduce.py
##########
@@ -376,6 +408,39 @@ def std(data, axis=None, keepdims=False, exclude=False):
return sqrt(_make._variance(data, m, axis, keepdims, exclude))
+def unbiased_std(data, axis=None, keepdims=False, exclude=False):
+ """Computes the unbiased standard deviation of data over given axes.
+
+ Parameters
+ ----------
+ data : relay.Expr
+ The input data
+
+ axis : None or int or tuple of int
+ Axis or axes along which a standard deviation operation is performed.
+ The default, axis=None, will compute the standard deviation of all elements in the
+ input array. If axis is negative it counts from the last to the first axis.
+
+ keepdims : bool
+ If this is set to True, the axes which are reduced are left in the result as dimensions
+ with size one.
+ With this option, the result will broadcast correctly against the input array.
+
+ exclude : bool
+ If `exclude` is true, reduction will be performed on the axes that are
+ NOT in axis instead.
+
+ Returns
+ -------
+ result : relay.Expr
+ The computed result.
+ """
+ axis = [axis] if isinstance(axis, int) else axis
+ m = mean(data, axis, True, exclude)
+
+ return sqrt(_make._unbiased_variance(data, m, axis, keepdims, exclude))
Review comment:
Understood. Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467661956
##########
File path: src/relay/op/tensor/reduce.cc
##########
@@ -266,7 +269,7 @@ bool ArgReduceRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
CHECK(param != nullptr);
// assign output type and shape
- auto oshape = ReduceShapeImpl(in_shape, param, reporter);
+ auto oshape = ReduceShapeImpl<ReduceAttrs>(in_shape, param, reporter);
Review comment:
no need to add explicit template annotation, it should be inferred
Safe for two occurences below
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#issuecomment-670850577
> I think adding `unbiased` argument to variance op will change the consistency. It will also bring more work to isolate the variance op from reduce ops.
I'm not completely sure what you mean by above, but how about adding `VarianceAttrs` with default `unbiased=false`? I think that should fix the coupling between variance and reduce op you are referring to?
Anyway, adding two new ops just for the sake of adding `unbiased` support sounds a bit overkill to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi merged pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] shiwenloong commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
shiwenloong commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467655004
##########
File path: include/tvm/relay/attrs/reduce.h
##########
@@ -60,6 +60,37 @@ struct ReduceAttrs : public tvm::AttrsNode<ReduceAttrs> {
"Whether to perform reduction on axis that are NOT in axis instead.");
}
};
+
+struct VarianceAttrs : public tvm::AttrsNode<VarianceAttrs> {
Review comment:
If `VarianceAttrs` is inherited from `ReduceAttrs`, `VarianceAttrs` varaibles in python can't visit `unbiased` because it is inherited from `tvm::AttrsNode<ReduceAttrs>` instead of `tvm::AttrsNode<VarianceAttrs>`. For example, `orig.attrs.unbiased` in `_tensor_grad.py:592` will be confronted with the bug.
https://github.com/apache/incubator-tvm/blob/375d377e6134e74ae6b7b489dde6182045b24774/python/tvm/relay/op/_tensor_grad.py#L588-L592
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] shiwenloong commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
shiwenloong commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467663242
##########
File path: src/relay/op/tensor/reduce.cc
##########
@@ -266,7 +269,7 @@ bool ArgReduceRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
CHECK(param != nullptr);
// assign output type and shape
- auto oshape = ReduceShapeImpl(in_shape, param, reporter);
+ auto oshape = ReduceShapeImpl<ReduceAttrs>(in_shape, param, reporter);
Review comment:
They have been fixed. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6232: [Relay][Op] Add unbiased variance op and corresponding support in pytorch frontend
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6232:
URL: https://github.com/apache/incubator-tvm/pull/6232#discussion_r467661956
##########
File path: src/relay/op/tensor/reduce.cc
##########
@@ -266,7 +269,7 @@ bool ArgReduceRel(const Array<Type>& types, int num_inputs, const Attrs& attrs,
CHECK(param != nullptr);
// assign output type and shape
- auto oshape = ReduceShapeImpl(in_shape, param, reporter);
+ auto oshape = ReduceShapeImpl<ReduceAttrs>(in_shape, param, reporter);
Review comment:
no need to add explicit template annotation, it should be inferred
Same for two occurences below
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org