[fix issue#9976] The assignment problem in NDArray #9981

wkcn · 2018-03-03T08:38:37Z

Description

>>> a = mx.nd.array(np.arange(12).reshape((3,4)))
>>> a

[[ 0.  1.  2.  3.]
 [ 4.  5.  6.  7.]
 [ 8.  9. 10. 11.]]
<NDArray 3x4 @cpu(0)>
>>> a[0]=a[1]     #   HERE
>>> a

[[ 0.  1.  2.  3.]
 [ 4.  5.  6.  7.]
 [ 8.  9. 10. 11.]]
<NDArray 3x4 @cpu(0)>

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

Add byte_offset() function for NDArray
Fix the assignment problem in NDArray. https://github.com/apache/incubator-mxnet/blob/master/src/ndarray/ndarray.cc#L1131

Comments

Is it necessary to add byte_offset() function for NDArray?
Is it necessary to add is_itself() function for NDArray?

marcoabreu · 2018-03-03T10:58:55Z

Hello, could you please add a test for this case?

wkcn · 2018-03-03T11:07:18Z

@marcoabreu Yes, I will add it soon :)

marcoabreu · 2018-03-03T11:54:49Z

@piiswrong

reminisce

The last three test cases are not affected by the bug. But there is nothing wrong to add them here.

reminisce · 2018-03-04T05:22:10Z

tests/python/unittest/test_ndarray.py

+ # assign directly
+ a_np[0] = a_np[1]
+ a_nd[0] = a_nd[1]
+ assert np.allclose(a_np, a_nd.asnumpy())


Use assert same(a_np, a_nd.asnumpy()). Same for all others.

Thank you! I will modify it.

reminisce · 2018-03-04T05:24:38Z

tests/python/unittest/test_ndarray.py

+ a_np[0] = a_np[1]
+ a_nd[0] = a_nd[1]
+ assert np.allclose(a_np, a_nd.asnumpy())
+ assert id(a_nd) == a_nd_id


What's the purpose of this check? This seems always true.

My mistake. I want to check the memory address of NDArray didn't change.
And assert np.allclose(a_np, a_nd.asnumpy()) means checking issue #9976.

reminisce · 2018-03-04T05:25:48Z

tests/python/unittest/test_ndarray.py

@@ -1099,6 +1099,39 @@ def test_assign_float_value_to_ndarray():
 b[0] = a[0]
 assert same(a, b.asnumpy())

+@with_seed()
+def test_ndarray_assignment():


Add issue link as code comment.

Change the function name to a special one since this is only for testing special cases. General cases have been covered by test_ndarray_indexing.

Okay. Thanks!

piiswrong · 2018-03-04T08:31:43Z

src/ndarray/ndarray.cc

@@ -1128,7 +1128,7 @@ void CopyFromToImpl(const NDArray& from, const NDArray& to,
 }

 void CopyFromTo(const NDArray& from, const NDArray& to, int priority) {
- if (from.var() == to.var()) {
+ if (from.var() == to.var() && from.byte_offset() == to.byte_offset()) {


should we also check for size? @reminisce

@piiswrong There is a shape equality check after this. For CopyFromTo, I think the added condition is sufficient. Agree we should also check sizes if it is for a function of checking if two ndarrays are the same one.

piiswrong · 2018-03-05T01:49:55Z

@marcoabreu retrigger test?

wkcn · 2018-03-05T01:53:18Z

@piiswrong Thank you! I will change the code and then trigger test.

…to fix_the_assignnment_problem_in_NDArray

…x the dtype in the test

marcoabreu · 2018-03-05T09:20:54Z

@piiswrong you can always retrigger a test by logging in at http://jenkins.mxnet-ci.amazon-ml.com/ (committers only)

* fix the assignment problem in NDArray * add ndarray assignment test * fix test_ndarray_assignment() * fix comparison precision for test_ndarray_assignment * rename test_ndarray_assignment to test_assign_a_row_to_ndarray and fix the dtype in the test

fix the assignment problem in NDArray

a78c4a8

wkcn requested a review from cjolivier01 as a code owner March 3, 2018 08:38

add ndarray assignment test

5ff921c

marcoabreu approved these changes Mar 3, 2018

View reviewed changes

wkcn added 2 commits March 3, 2018 12:45

fix test_ndarray_assignment()

e33fd26

fix comparison precision for test_ndarray_assignment

02753dd

reminisce reviewed Mar 4, 2018

View reviewed changes

piiswrong reviewed Mar 4, 2018

View reviewed changes

wkcn added 2 commits March 5, 2018 02:05

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

5e5938d

…to fix_the_assignnment_problem_in_NDArray

rename test_ndarray_assignment to test_assign_a_row_to_ndarray and fi…

ed90fd6

…x the dtype in the test

piiswrong merged commit e2b1a56 into apache:master Mar 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix issue#9976] The assignment problem in NDArray #9981

[fix issue#9976] The assignment problem in NDArray #9981

wkcn commented Mar 3, 2018 •

edited

Loading

marcoabreu commented Mar 3, 2018

wkcn commented Mar 3, 2018

marcoabreu commented Mar 3, 2018

reminisce left a comment

reminisce Mar 4, 2018

wkcn Mar 5, 2018

reminisce Mar 4, 2018

wkcn Mar 5, 2018

reminisce Mar 4, 2018

wkcn Mar 5, 2018

piiswrong Mar 4, 2018

reminisce Mar 4, 2018

piiswrong commented Mar 5, 2018

wkcn commented Mar 5, 2018

marcoabreu commented Mar 5, 2018

[fix issue#9976] The assignment problem in NDArray #9981

[fix issue#9976] The assignment problem in NDArray #9981

Conversation

wkcn commented Mar 3, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

marcoabreu commented Mar 3, 2018

wkcn commented Mar 3, 2018

marcoabreu commented Mar 3, 2018

reminisce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piiswrong commented Mar 5, 2018

wkcn commented Mar 5, 2018

marcoabreu commented Mar 5, 2018

wkcn commented Mar 3, 2018 •

edited

Loading