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

eye operator, for default storage type #9770

Merged
merged 5 commits into from
Feb 26, 2018
Merged

eye operator, for default storage type #9770

merged 5 commits into from
Feb 26, 2018

Conversation

ZiyueHuang
Copy link
Member

@ZiyueHuang ZiyueHuang commented Feb 12, 2018

Description

eye operator, for default storage type

Requested in https://discuss.mxnet.io/t/is-there-an-eye-function-in-the-ndarray-api/526/5

cc @eric-haibin-lin

Checklist

Essentials

  • Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • eye, unittest

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@@ -736,6 +736,14 @@ def test_output():
assert_almost_equal(out.asnumpy(), ones.asnumpy() * 2)
arange_out = mx.nd.arange(0, 20, dtype='int64')
assert_almost_equal(arange_out.asnumpy(), np.arange(0, 20))
N_array = np.random.randint(1, high=3, size=3)
M_array = np.random.randint(1, high=3, size=3)
k_array = np.random.randint(-5, high=5, size=3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you run some more trials?
At least locally & with larger ranges. Just to verify if there are edge cases

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think there is the case where M is 0

template<typename DType>
MSHADOW_XINLINE static void Map(int i, DType* out_data, const nnvm::dim_t num_cols,
const nnvm::dim_t k) {
if ((i % num_cols) == ((i / num_cols) + k)) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like two divides per value-fill. Would it be faster ( at least for CPU), to fill with 0's and then "walk" across an offset (using only add) to fill in the one's?

@piiswrong
Copy link
Contributor

@ZiyueHuang ping

@ZiyueHuang
Copy link
Member Author

Thanks for your comments. Sorry for the late response.

@piiswrong I increased the size and the number of trials and added M=0 case in unittest. I also run 1000 times locally(ubuntu).

@cjolivier01 I changed to two kernels. About 10 times faster on cpu.

if (nnz > 0) {
Kernel<eye_dns_fill<req_type>, xpu>::Launch(
s, nnz, out_data.dptr<DType>(),
std::max((nnvm::dim_t)0, param.k), param.k, 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.

nit:

static_cast<nnvm::dim_t>(0)

or

0LL

although the latter sort of ties dim_t to a specific type.

@piiswrong piiswrong merged commit db24ac1 into apache:master Feb 26, 2018
@ZiyueHuang ZiyueHuang deleted the dense_eye branch May 3, 2018 15:30
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
* eye

* more test

* change to two kernels

* address comments

* Update init_op.h
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
* eye

* more test

* change to two kernels

* address comments

* Update init_op.h
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.

3 participants