Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Operators for mean(csr, axis=0) and mean(csr, axis=1) #8264

Merged
merged 22 commits into from
Nov 8, 2017

Conversation

anirudh2290
Copy link
Member

@anirudh2290 anirudh2290 commented Oct 13, 2017

Description

Checklist

Essentials

  • Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • For user-facing API changes, API doc string has been updated.
  • To my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • mx.nd.mean(axis=0), tests, (and when applicable, API doc)
  • mx.nd.mean(axis=1), tests, (and when applicable, API doc)

Comments

@piiswrong @eric-haibin-lin @reminisce @cjolivier01

@piiswrong
Copy link
Contributor

I just merged #8174, what's in this PR?

@anirudh2290
Copy link
Member Author

@piiswrong this PR doesn't have to go in this release. This just adds support for doing mean operation along the axis. The logic is small but it still needs to go through review.

@@ -566,7 +566,7 @@ struct SumCsrKernel<req, 1> {
}
};

template <typename xpu>
template <typename xpu, bool normalize = false>
Copy link
Member

Choose a reason for hiding this comment

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

Please add brief comment regarding what normalize is for

Copy link
Member Author

Choose a reason for hiding this comment

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

Added comment!

if (normalize) {
mshadow::Tensor<xpu, 1, DType> out_data =
output->data().FlatTo1D<xpu, DType>(s);
out_data /= scalar<DType>(num_cols);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using mshadow here for the divide, can you use a divide by scalar kernel? Mshadow is being deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

You can use:
Kernel<mxnet_op::op_with_req<mshadow::op__div, Req>, xpu>::Launch(...)

Dee _div_scalar op (elemwise_binary_scalar_op_basic.cc)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Chris! I have fixed this and I am using Kernel::Launch instead of mshadow.

@@ -96,8 +96,11 @@ MXNET_OPERATOR_REGISTER_REDUCE_BACKWARD(_backward_sum)
.set_attr<FCompute>("FCompute<cpu>", ReduceAxesBackwardUseNone<cpu>);

MXNET_OPERATOR_REGISTER_REDUCE(mean)
MXNET_ADD_SPARSE_OP_ALIAS(mean)
Copy link
Member

Choose a reason for hiding this comment

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

Does it work for GPU?

Copy link
Member Author

Choose a reason for hiding this comment

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

It falls back to dense operator for GPU.

Kernel<SumCsrKernel<req_type, 1>, xpu>::Launch(
s, out_data_size, output->data().dptr<DType>(), in_indptr,
in_data);
MSHADOW_IDX_TYPE_SWITCH(input.aux_type(kIdx), IType, {
Copy link
Member

Choose a reason for hiding this comment

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

If the mean turns out to be zero, does the op effectively produce a dense result? What does numpy produce in an equivalent dense operation?

Copy link
Member Author

Choose a reason for hiding this comment

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

The operator is mean(csr, axis=0) = dense and thus always produces a dense result.

@piiswrong piiswrong merged commit c2358f9 into apache:master Nov 8, 2017
cjolivier01 pushed a commit to cjolivier01/mxnet that referenced this pull request Nov 9, 2017
* Add Infer storage for sparse slice operator

* Remove unused files

* Indentation fix and add gpu test for fallback

* Change sum builtin to py_sum

* Add sum_axis(csr,axis=0)=dense and sum(csr,axis=1)=dense operator

* Documentation changes for sparse

* Add fallback unittest for keepdims and exclude

* PR review based changes
:

* Fix CHECK_NE

* Change in_stype to int

* Using const int instead of int

* Initialize mid with the start

* Add mean(csr, axis=0) and mean(csr, axis=1)

* Removing whitespace

* Add brief comment for normalize

* Add a comma

* Lint changes
eric-haibin-lin pushed a commit to eric-haibin-lin/mxnet that referenced this pull request Dec 3, 2017
* Add Infer storage for sparse slice operator

* Remove unused files

* Indentation fix and add gpu test for fallback

* Change sum builtin to py_sum

* Add sum_axis(csr,axis=0)=dense and sum(csr,axis=1)=dense operator

* Documentation changes for sparse

* Add fallback unittest for keepdims and exclude

* PR review based changes
:

* Fix CHECK_NE

* Change in_stype to int

* Using const int instead of int

* Initialize mid with the start

* Add mean(csr, axis=0) and mean(csr, axis=1)

* Removing whitespace

* Add brief comment for normalize

* Add a comma

* Lint changes
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
* Add Infer storage for sparse slice operator

* Remove unused files

* Indentation fix and add gpu test for fallback

* Change sum builtin to py_sum

* Add sum_axis(csr,axis=0)=dense and sum(csr,axis=1)=dense operator

* Documentation changes for sparse

* Add fallback unittest for keepdims and exclude

* PR review based changes
:

* Fix CHECK_NE

* Change in_stype to int

* Using const int instead of int

* Initialize mid with the start

* Add mean(csr, axis=0) and mean(csr, axis=1)

* Removing whitespace

* Add brief comment for normalize

* Add a comma

* Lint changes
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants