8000 ENH: Make layout order an initialization parameter of ArrayProxy by effigies · Pull Request #1131 · nipy/nibabel · GitHub
[go: up one dir, main page]

Skip to content

ENH: Make layout order an initialization parameter of ArrayProxy #1131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Sep 7, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
RF: Change ArrayProxy.order class var to _default_order, deprecate order
  • Loading branch information
effigies committed Aug 31, 2022
commit 3cfe3370e5f2dfd55e175553af70ebb1f3d5a87b
19 changes: 16 additions & 3 deletions nibabel/arrayproxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"""
from contextlib import contextmanager
from threading import RLock
import warnings

import numpy as np

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

I forget the mechanism to time out the deprecation warning ... but shouldn't we put that in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a decorator that works for deprecating an entire function/method/class. It does not work for cases like this. This is why I added the check below:

# 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 = \
Expand Down
39 changes: 39 additions & 0 deletions nibabel/tests/test_arrayproxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -57,6 +60,10 @@ def copy(self):

class CArrayProxy(ArrayProxy):
# C array memory layout
_default_order = 'C'


class DeprecatedCArrayProxy(ArrayProxy):
Copy link
Member

Choose a reason for hiding this comment

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

Timed out depraction again (as above)?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is specifically for testing. The warning/expiration will show up in test_deprecated_order_classvar. When we remove that function, we can remove this class. I'll add a comment...

order = 'C'


Expand Down Expand Up @@ -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()
Comment on lines +231 to +235
Copy link
Member Author

Choose a reason for hiding this comment

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

... here.


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))
Expand Down
0