apache / mxnet

Lightweight, Portable, Flexible Distributed/Mobile Deep Learning with Dynamic, Mutation-aware Dataflow Dep Scheduler; for Python, R, Julia, Scala, Go, Javascript and more
https://mxnet.apache.org
Apache License 2.0
20.78k stars 6.79k forks source link

Inconsistent behavior of NDArray Indexing #9130

Open safrooze opened 6 years ago

safrooze commented 6 years ago

Description

NDArray.__getitem__ has an undocumented and inconsistent behavior with respect to creating a copy or not. Depending on how indexing is called, it may return a pointer to the current buffer or may create a new buffer.

Environment info (Required)

----------Python Info----------
Version      : 3.6.3
Compiler     : GCC 4.2.1 Compatible Clang 4.0.1 (tags/RELEASE_401/final)
Build        : ('default', 'Nov  8 2017 18:10:31')
Arch         : ('64bit', '')
------------Pip Info-----------
Version      : 9.0.1
Directory    : /Users/safrooze/tools/anaconda3/envs/ipy3/lib/python3.6/site-packages/pip
----------MXNet Info-----------
Version      : 1.0.0
Directory    : /Users/safrooze/tools/anaconda3/envs/ipy3/lib/python3.6/site-packages/mxnet
Commit Hash   : 0f05c65492e38f89012e5595c6d60bb67e2d418f
----------System Info----------
Platform     : Darwin-16.7.0-x86_64-i386-64bit
system       : Darwin
node         : 8c85904c8831.ant.amazon.com
release      : 16.7.0
version      : Darwin Kernel Version 16.7.0: Wed Oct  4 00:17:00 PDT 2017; root:xnu-3789.71.6~1/RELEASE_X86_64
----------Hardware Info----------
machine      : x86_64
processor    : i386
b'machdep.cpu.extfeatures: SYSCALL XD 1GBPAGE EM64T LAHF LZCNT PREFETCHW RDTSCP TSCI'
b'machdep.cpu.leaf7_features: SMEP ERMS RDWRFSGS TSC_THREAD_OFFSET BMI1 HLE AVX2 BMI2 INVPCID RTM SMAP RDSEED ADX IPT SGX FPU_CSDS MPX CLFSOPT'
b'machdep.cpu.features: FPU VME DE PSE TSC MSR PAE MCE CX8 APIC SEP MTRR PGE MCA CMOV PAT PSE36 CLFSH DS ACPI MMX FXSR SSE SSE2 SS HTT TM PBE SSE3 PCLMULQDQ DTES64 MON DSCPL VMX SMX EST TM2 SSSE3 FMA CX16 TPR PDCM SSE4.1 SSE4.2 x2APIC MOVBE POPCNT AES PCID XSAVE OSXSAVE SEGLIM64 TSCTMR AVX1.0 RDRAND F16C'
b'machdep.cpu.brand_string: Intel(R) Core(TM) i7-7660U CPU @ 2.50GHz'
----------Network Test----------
Setting timeout: 10
Timing for MXNet: https://github.com/apache/incubator-mxnet, DNS: 0.0649 sec, LOAD: 0.7477 sec.
Timing for Gluon Tutorial(en): http://gluon.mxnet.io, DNS: 0.2182 sec, LOAD: 0.1788 sec.
Timing for Gluon Tutorial(cn): https://zh.gluon.ai, DNS: 0.1070 sec, LOAD: 0.3729 sec.
Timing for FashionMNIST: https://apache-mxnet.s3-accelerate.dualstack.amazonaws.com/gluon/dataset/fashion-mnist/train-labels-idx1-ubyte.gz, DNS: 0.0726 sec, LOAD: 0.4160 sec.
Timing for PYPI: https://pypi.python.org/pypi/pip, DNS: 0.0596 sec, LOAD: 0.4250 sec.
Timing for Conda: https://repo.continuum.io/pkgs/free/, DNS: 0.0612 sec, LOAD: 0.6522 sec.

Package used (Python/R/Scala/Julia): Python

Minimum reproducible example

a=mx.nd.ones((3,3))
b=mx.nd.ones((3,3))
a_sliced=a[0:1]
b_sliced=b[0:1, :]
a_sliced*=10
b_sliced*=10
print("a",a)
print("b", b)

The above code results in the following output:

a 
[[ 10.  10.  10.]
 [  1.   1.   1.]
 [  1.   1.   1.]]
<NDArray 3x3 @cpu(0)>

b 
[[ 1.  1.  1.]
 [ 1.  1.  1.]
 [ 1.  1.  1.]]
<NDArray 3x3 @cpu(0)>

For the record, I also tried NDArray.slice() and mx.nd.slice() calls and they both always create a copy instead of returning a pointer to the existing buffer.

szha commented 6 years ago

@reminisce

reminisce commented 6 years ago

For efficient indexing, __getitem__ slicing would create a view if the sliced elements are contiguous in memory, or a copy if the sliced elements are not contiguous. It's determined by the fact that we only handle NDArrays with contiguous memory allocation in MXNet.

Operator slice in imperative mode always returns a copy like most of the other operators.

We can make it clear in the docstring of __getitem__.

safrooze commented 6 years ago

That's exactly what I thought is happening. Although keeping efficiency in mind is always great, I see two problems:

  1. There is a bug in detecting whether the result of slice is contiguous or not, as evident in my example. There is no difference between indexing [0:1] and indexing [0:1, :]. The code assumes if you pass in the second dimension it won't be contiguous.
  2. Even with proper documentation and correct contiguous buffer detection, I feel the behavior is too ambiguous. As a user you don't know what you'll get! Is it a copy or should I create a copy to make sure I'm not modifying the original array? This can make the overall code even less efficient. Most people may be surprised with the copy implementation as the name and API both imply Numpy behavior.
reminisce commented 6 years ago

Agree it's not a good user experience. Let's add it as a future improvement.