Merge pull request #1131 from effigies/enh/arrayproxy_order

ENH: Make layout order an initialization parameter of ArrayProxy

nibabel/arrayproxy.py

@@ -27,6 +27,7 @@
 """
 from contextlib import contextmanager
 from threading import RLock
+import warnings
 
 import numpy as np
 
@@ -53,7 +54,7 @@
 KEEP_FILE_OPEN_DEFAULT = False
 
 
-class ArrayProxy(object):
+class ArrayProxy:
     """ Class to act as proxy for the array that can be read from a file
 
     The array proxy allows us to freeze the passed fileobj and header such that
@@ -83,10 +84,9 @@ class ArrayProxy(object):
     See :mod:`nibabel.minc1`, :mod:`nibabel.ecat` and :mod:`nibabel.parrec` for
     examples.
     """
-    # Assume Fortran array memory layout
-    order = 'F'
+    _default_order = 'F'
 
-    def __init__(self, file_like, spec, *, mmap=True, keep_file_open=None):
+    def __init__(self, file_like, spec, *, mmap=True, order=None, keep_file_open=None):
         """Initialize array proxy instance
 
         Parameters
@@ -116,6 +116,11 @@ def __init__(self, file_like, spec, *, mmap=True, keep_file_open=None):
             True gives the same behavior as ``mmap='c'``.  If `file_like`
             cannot be memory-mapped, ignore `mmap` value and read array from
             file.
+        order : {None, 'F', 'C'}, optional, keyword only
+            `order` controls the order of the data array layout. Fortran-style,
+            column-major order may be indicated with 'F', and C-style, row-major
+            order may be indicated with 'C'. None gives the default order, that
+            comes from the `_default_order` class variable.
         keep_file_open : { None, True, False }, optional, keyword only
             `keep_file_open` controls whether a new file handle is created
             every time the image is accessed, or a single file handle is
@@ -128,6 +133,8 @@ def __init__(self, file_like, spec, *, mmap=True, keep_file_open=None):
         """
         if mmap not in (True, False, 'c', 'r'):
             raise ValueError("mmap should be one of {True, False, 'c', 'r'}")
+        if order not in (None, 'C', 'F'):
+            raise ValueError("order should be one of {None, 'C', 'F'}")
         self.file_like = file_like
         if hasattr(spec, 'get_data_shape'):
             slope, inter = spec.get_slope_inter()
@@ -142,11 +149,25 @@ def __init__(self, file_like, spec, *, mmap=True, keep_file_open=None):
         else:
             raise TypeError('spec must be tuple of length 2-5 or header object')
 
+        # Warn downstream users that the class variable order is going away
+        if hasattr(self.__class__, 'order'):
+            warnings.warn(f'Class {self.__class__} has an `order` class variable. '
+                          'ArrayProxy subclasses should rename this variable to `_default_order` '
+                          'to avoid conflict with instance variables.\n'
+                          '* deprecated in version: 5.0\n'
+                          '* will raise error in version: 7.0\n',
+                          DeprecationWarning, stacklevel=2)
+            # Override _default_order with order, to follow intent of subclasser
+            self._default_order = self.order
+
         # Copies of values needed to read array
         self._shape, self._dtype, self._offset, self._slope, self._inter = par
         # Permit any specifier that can be interpreted as a numpy dtype
         self._dtype = np.dtype(self._dtype)
         self._mmap = mmap
+        if order is None:
+            order = self._default_order
+        self.order = order
         # Flags to keep track of whether a single ImageOpener is created, and
         # whether a single underlying file handle is created.
         self._keep_file_open, self._persist_opener = \

nibabel/tests/test_arrayproxy.py

@@ -15,13 +15,16 @@
 
 import pickle
 from io import BytesIO
+from packaging.version import Version
 from ..tmpdirs import InTemporaryDirectory
 
 import numpy as np
 
+from .. import __version__
 from ..arrayproxy import (ArrayProxy, is_proxy, reshape_dataobj, get_obj_dtype)
 from ..openers import ImageOpener
 from ..nifti1 import Nifti1Header
+from ..deprecator import ExpiredDeprecationError
 
 from unittest import mock
 
@@ -57,6 +60,11 @@ def copy(self):
 
 class CArrayProxy(ArrayProxy):
     # C array memory layout
+    _default_order = 'C'
+
+
+class DeprecatedCArrayProxy(ArrayProxy):
+    # Used in test_deprecated_order_classvar. Remove when that test is removed (8.0)
     order = 'C'
 
 
@@ -81,6 +89,9 @@ def test_init():
     assert ap.shape != shape
     # Data stays the same, also
     assert_array_equal(np.asarray(ap), arr)
+    # You wouldn't do this, but order=None explicitly requests the default order
+    ap2 = ArrayProxy(bio, FunkyHeader(arr.shape), order=None)
+    assert_array_equal(np.asarray(ap2), arr)
     # C order also possible
     bio = BytesIO()
     bio.seek(16)
@@ -90,6 +101,8 @@ def test_init():
     # Illegal init
     with pytest.raises(TypeError):
         ArrayProxy(bio, object())
+    with pytest.raises(ValueError):
+        ArrayProxy(bio, hdr, order='badval')
 
 
 def test_tuplespec():
@@ -154,33 +167,87 @@ def test_nifti1_init():
         assert_array_equal(np.asarray(ap), arr * 2.0 + 10)
 
 
-def test_proxy_slicing():
-    shapes = (15, 16, 17)
-    for n_dim in range(1, len(shapes) + 1):
-        shape = shapes[:n_dim]
-        arr = np.arange(np.prod(shape)).reshape(shape)
-        for offset in (0, 20):
-            hdr = Nifti1Header()
-            hdr.set_data_offset(offset)
-            hdr.set_data_dtype(arr.dtype)
-            hdr.set_data_shape(shape)
-            for order, klass in ('F', ArrayProxy), ('C', CArrayProxy):
-                fobj = BytesIO()
-                fobj.write(b'\0' * offset)
-                fobj.write(arr.tobytes(order=order))
-                prox = klass(fobj, hdr)
-                for sliceobj in slicer_samples(shape):
-                    assert_array_equal(arr[sliceobj], prox[sliceobj])
-    # Check slicing works with scaling
+@pytest.mark.parametrize("n_dim", (1, 2, 3))
+@pytest.mark.parametrize("offset", (0, 20))
+def test_proxy_slicing(n_dim, offset):
+    shape = (15, 16, 17)[:n_dim]
+    arr = np.arange(np.prod(shape)).reshape(shape)
+    hdr = Nifti1Header()
+    hdr.set_data_offset(offset)
+    hdr.set_data_dtype(arr.dtype)
+    hdr.set_data_shape(shape)
+    for order, klass in ('F', ArrayProxy), ('C', CArrayProxy):
+        fobj = BytesIO()
+        fobj.write(b'\0' * offset)
+        fobj.write(arr.tobytes(order=order))
+        prox = klass(fobj, hdr)
+        assert prox.order == order
+        for sliceobj in slicer_samples(shape):
+            assert_array_equal(arr[sliceobj], prox[sliceobj])
+
+
+def test_proxy_slicing_with_scaling():
+    shape = (15, 16, 17)
+    offset = 20
+    arr = np.arange(np.prod(shape)).reshape(shape)
+    hdr = Nifti1Header()
+    hdr.set_data_offset(offset)
+    hdr.set_data_dtype(arr.dtype)
+    hdr.set_data_shape(shape)
     hdr.set_slope_inter(2.0, 1.0)
     fobj = BytesIO()
-    fobj.write(b'\0' * offset)
+    fobj.write(bytes(offset))
     fobj.write(arr.tobytes(order='F'))
     prox = ArrayProxy(fobj, hdr)
     sliceobj = (None, slice(None), 1, -1)
     assert_array_equal(arr[sliceobj] * 2.0 + 1.0, prox[sliceobj])
 
 
+@pytest.mark.parametrize("order", ("C", "F"))
+def test_order_override(order):
+    shape = (15, 16, 17)
+    arr = np.arange(np.prod(shape)).reshape(shape)
+    fobj = BytesIO()
+    fobj.write(arr.tobytes(order=order))
+    for klass in (ArrayProxy, CArrayProxy):
+        prox = klass(fobj, (shape, arr.dtype), order=order)
+        assert prox.order == order
+        sliceobj = (None, slice(None), 1, -1)
+        assert_array_equal(arr[sliceobj], prox[sliceobj])
+
+
+def test_deprecated_order_classvar():
+    shape = (15, 16, 17)
+    arr = np.arange(np.prod(shape)).reshape(shape)
+    fobj = BytesIO()
+    fobj.write(arr.tobytes(order='C'))
+    sliceobj = (None, slice(None), 1, -1)
+
+    # We don't really care about the original order, just that the behavior
+    # of the deprecated mode matches the new behavior
+    fprox = ArrayProxy(fobj, (shape, arr.dtype), order='F')
+    cprox = ArrayProxy(fobj, (shape, arr.dtype), order='C')
+
+    # Start raising errors when we crank the dev version
+    if Version(__version__) >= Version('7.0.0.dev0'):
+        cm = pytest.raises(ExpiredDeprecationError)
+    else:
+        cm = pytest.deprecated_call()
+
+    with cm:
+        prox = DeprecatedCArrayProxy(fobj, (shape, arr.dtype))
+        assert prox.order == 'C'
+        assert_array_equal(prox[sliceobj], cprox[sliceobj])
+    with cm:
+        prox = DeprecatedCArrayProxy(fobj, (shape, arr.dtype), order='C')
+        assert prox.order == 'C'
+        assert_array_equal(prox[sliceobj], cprox[sliceobj])
+    with cm:
+        prox = DeprecatedCArrayProxy(fobj, (shape, arr.dtype), order='F')
+        assert prox.order == 'F'
+        assert_array_equal(prox[sliceobj], fprox[sliceobj])
+
+
 def test_is_proxy():
     # Test is_proxy function
     hdr = FunkyHeader((2, 3, 4))