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

[MXNET-294] Fix element wise multiply for csr ndarrays #10452

Merged
merged 8 commits into from
Apr 10, 2018

Conversation

rahul003
Copy link
Member

@rahul003 rahul003 commented Apr 7, 2018

Description

Fixes Issue #10431
JIRA

Tensor value was being used without initialization causing garbage values to crop up and make the product wrong.

First use with +=
https://github.com/apache/incubator-mxnet/blob/6f6a1dc4d350dcec3058abbe4ca5123b5fce536e/src/operator/tensor/elemwise_binary_op-inl.h#L339

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • 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
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Initialize tensor

Comments

  • There is already a test for this.

@eric-haibin-lin @anirudh2290 @cjolivier01 Please review

@anirudh2290
Copy link
Member

Can you add a test which calls elemwise_mul with same ndarray ?

@rahul003
Copy link
Member Author

rahul003 commented Apr 8, 2018

Thanks, added

if (!same_lhs_rhs) {
OpBase::FillDense<DType>(s, rhs_row.shape_.Size(), DType(0), req, rhs_row.dptr_);
}
OpBase::FillDense<DType>(s, rhs_row.shape_.Size(), DType(0), req, rhs_row.dptr_);
Copy link
Member

Choose a reason for hiding this comment

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

if its the same_lhs_rhs can we point to the same tensor for both lhs and rhs instead of calling filldense for rhs ?

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 for the suggestion, changed

@anirudh2290
Copy link
Member

Seems like a flaky test, opened an issue for the same: #10476. Can you please retrigger.

@rahul003
Copy link
Member Author

@anirudh2290 Build passed

@anirudh2290
Copy link
Member

@eric-haibin-lin can this be merged ?

@eric-haibin-lin eric-haibin-lin merged commit 8b172d7 into apache:master Apr 10, 2018
@rahul003 rahul003 deleted the csr-elem-mul branch April 12, 2018 02:40
rahul003 added a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
* initialize rhs

* add test for elemwise op on same array

* dont declare memory for rhs if same array

* dont declare memory for rhs if same array

* assign lhs to rhs if same arr

* whitespace changes

* lint fix

* trigger ci
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
* initialize rhs

* add test for elemwise op on same array

* dont declare memory for rhs if same array

* dont declare memory for rhs if same array

* assign lhs to rhs if same arr

* whitespace changes

* lint fix

* trigger ci
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