From ea68dd1697a896490d9131f540956eab8d30beb3 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Sat, 20 Aug 2022 09:24:56 -0400 Subject: [PATCH 1/8] ENH: Make layout order an initialization parameter of ArrayProxy --- nibabel/arrayproxy.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/nibabel/arrayproxy.py b/nibabel/arrayproxy.py index 6cda3a206a..9546360ca7 100644 --- a/nibabel/arrayproxy.py +++ b/nibabel/arrayproxy.py @@ -53,7 +53,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 +83,9 @@ class ArrayProxy(object): See :mod:`nibabel.minc1`, :mod:`nibabel.ecat` and :mod:`nibabel.parrec` for examples. """ - # Assume Fortran array memory layout 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 +115,10 @@ 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 : {'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'. The default order is 'F'. 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 +131,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 {'C', 'F'}") self.file_like = file_like if hasattr(spec, 'get_data_shape'): slope, inter = spec.get_slope_inter() @@ -147,6 +152,8 @@ def __init__(self, file_like, spec, *, mmap=True, keep_file_open=None): # Permit any specifier that can be interpreted as a numpy dtype self._dtype = np.dtype(self._dtype) self._mmap = mmap + if order is not None: + 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 = \ From 015559845a2217204e546d797a9f8eb25fe9e54d Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Sat, 20 Aug 2022 12:53:47 -0400 Subject: [PATCH 2/8] TEST: Refactor test loops as parameterizations --- nibabel/tests/test_arrayproxy.py | 47 +++++++++++++++++++------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/nibabel/tests/test_arrayproxy.py b/nibabel/tests/test_arrayproxy.py index 80806cae3a..deaa6f4e11 100644 --- a/nibabel/tests/test_arrayproxy.py +++ b/nibabel/tests/test_arrayproxy.py @@ -154,27 +154,36 @@ 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) From 47209327e6f9da864d5f88facd2a84aa1a853a38 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Sat, 20 Aug 2022 12:54:05 -0400 Subject: [PATCH 3/8] TEST: Test order kwarg to ArrayProxy classes --- nibabel/tests/test_arrayproxy.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/nibabel/tests/test_arrayproxy.py b/nibabel/tests/test_arrayproxy.py index deaa6f4e11..c4c44f72f2 100644 --- a/nibabel/tests/test_arrayproxy.py +++ b/nibabel/tests/test_arrayproxy.py @@ -190,6 +190,19 @@ def test_proxy_slicing_with_scaling(): 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_is_proxy(): # Test is_proxy function hdr = FunkyHeader((2, 3, 4)) From df252cad2cbb4cbcb7624674d54c2460715baf81 Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Tue, 30 Aug 2022 16:12:31 -0400 Subject: [PATCH 4/8] Update nibabel/arrayproxy.py Co-authored-by: Matthew Brett --- nibabel/arrayproxy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nibabel/arrayproxy.py b/nibabel/arrayproxy.py index 9546360ca7..230ad00272 100644 --- a/nibabel/arrayproxy.py +++ b/nibabel/arrayproxy.py @@ -115,7 +115,7 @@ def __init__(self, file_like, spec, *, mmap=True, order=None, keep_file_open=Non True gives the same behavior as ``mmap='c'``. If `file_like` cannot be memory-mapped, ignore `mmap` value and read array from file. - order : {'F', 'C'}, optional, keyword only + 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'. The default order is 'F'. From 3cfe3370e5f2dfd55e175553af70ebb1f3d5a87b Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Tue, 30 Aug 2022 17:08:38 -0400 Subject: [PATCH 5/8] RF: Change ArrayProxy.order class var to _default_order, deprecate order --- nibabel/arrayproxy.py | 19 +++++++++++++--- nibabel/tests/test_arrayproxy.py | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/nibabel/arrayproxy.py b/nibabel/arrayproxy.py index 230ad00272..4b8287194e 100644 --- a/nibabel/arrayproxy.py +++ b/nibabel/arrayproxy.py @@ -27,6 +27,7 @@ """ from contextlib import contextmanager from threading import RLock +import warnings import numpy as np @@ -83,7 +84,7 @@ class ArrayProxy: See :mod:`nibabel.minc1`, :mod:`nibabel.ecat` and :mod:`nibabel.parrec` for examples. """ - order = 'F' + _default_order = 'F' def __init__(self, file_like, spec, *, mmap=True, order=None, keep_file_open=None): """Initialize array proxy instance @@ -147,13 +148,25 @@ def __init__(self, file_like, spec, *, mmap=True, order=None, keep_file_open=Non 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 not None: - self.order = order + 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 = \ diff --git a/nibabel/tests/test_arrayproxy.py b/nibabel/tests/test_arrayproxy.py index c4c44f72f2..4bbbe31abd 100644 --- a/nibabel/tests/test_arrayproxy.py +++ b/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,10 @@ def copy(self): class CArrayProxy(ArrayProxy): # C array memory layout + _default_order = 'C' + + +class DeprecatedCArrayProxy(ArrayProxy): order = 'C' @@ -203,6 +210,38 @@ def test_order_override(order): 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)) From dcf95664710feebd0a18aed305626a5ecb733c67 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Wed, 31 Aug 2022 09:54:02 -0400 Subject: [PATCH 6/8] TEST: Check None and invalid order arguments --- nibabel/tests/test_arrayproxy.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nibabel/tests/test_arrayproxy.py b/nibabel/tests/test_arrayproxy.py index 4bbbe31abd..fdf807654a 100644 --- a/nibabel/tests/test_arrayproxy.py +++ b/nibabel/tests/test_arrayproxy.py @@ -88,6 +88,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) @@ -97,6 +100,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(): From cba4607579acea6f7f2d7ab8e8d1507d56ab2344 Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Wed, 7 Sep 2022 08:41:35 -0400 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Matthew Brett --- nibabel/arrayproxy.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/nibabel/arrayproxy.py b/nibabel/arrayproxy.py index 4b8287194e..dc9b171c0b 100644 --- a/nibabel/arrayproxy.py +++ b/nibabel/arrayproxy.py @@ -119,7 +119,8 @@ def __init__(self, file_like, spec, *, mmap=True, order=None, keep_file_open=Non 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'. The default order is 'F'. + 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 @@ -133,7 +134,7 @@ def __init__(self, file_like, spec, *, mmap=True, order=None, keep_file_open=Non 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 {'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() From cff42d6cd971803fc774ec69db3331a45c731496 Mon Sep 17 00:00:00 2001 From: Chris Markiewicz Date: Wed, 7 Sep 2022 08:46:14 -0400 Subject: [PATCH 8/8] Update nibabel/tests/test_arrayproxy.py --- nibabel/tests/test_arrayproxy.py | 1 + 1 file changed, 1 insertion(+) diff --git a/nibabel/tests/test_arrayproxy.py b/nibabel/tests/test_arrayproxy.py index fdf807654a..ed89105aa0 100644 --- a/nibabel/tests/test_arrayproxy.py +++ b/nibabel/tests/test_arrayproxy.py @@ -64,6 +64,7 @@ class CArrayProxy(ArrayProxy): class DeprecatedCArrayProxy(ArrayProxy): + # Used in test_deprecated_order_classvar. Remove when that test is removed (8.0) order = 'C'