ENH: ArrayProxy reshape #521

effigies · 2017-03-25T01:46:13Z

Might as well start a PR. Are there any reasons this is a terrible idea?

Closes #520.

(Will close this if it doesn't actually resolve the issue.)

coveralls · 2017-03-25T02:03:40Z

Coverage decreased (-0.003%) to 96.154% when pulling 8b41f55 on effigies:ap_reshape into ca977ab on nipy:master.

codecov-io · 2017-03-25T02:13:12Z

Codecov Report

Merging #521 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #521      +/-   ##
==========================================
+ Coverage   94.14%   94.17%   +0.03%     
==========================================
  Files         175      175              
  Lines       23890    23930      +40     
  Branches     2568     2574       +6     
==========================================
+ Hits        22492    22537      +45     
+ Misses        918      916       -2     
+ Partials      480      477       -3

Impacted Files	Coverage Δ
nibabel/cifti2/parse_cifti2.py	`83.76% <ø> (+0.8%)`	⬆️
nibabel/arrayproxy.py	`100% <100%> (ø)`	⬆️
nibabel/tests/test_proxy_api.py	`98.63% <100%> (ø)`	⬆️
nibabel/cifti2/tests/test_cifti2io.py	`98.29% <100%> (ø)`	⬆️
nibabel/tests/test_arrayproxy.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8de91a1...37405c3. Read the comment docs.

demianw · 2017-03-25T08:46:04Z

This seems to solve the issue!
Just that, to close #520
we need to also include this patch

--- a/nibabel/cifti2/parse_cifti2.py
+++ b/nibabel/cifti2/parse_cifti2.py
@@ -147,7 +147,6 @@ class _Cifti2AsNiftiImage(Nifti2Image):
         if self.cifti_img is None:
             raise ValueError('Nifti2 header does not contain a CIFTI2 '
                              'extension')
-        self.cifti_img.data = self.get_data()


 class Cifti2Parser(xml.XmlParser):

matthew-brett · 2017-03-25T09:24:19Z

Thanks for taking the initiative on this.

I've been thinking about this for a bit.

Reshape is a good idea in general.

The immediate problem is that we can expect the headers to be pretty big, and the ArrayProxy constructor copies them.

More generally, I would really like to add an alternative constructor signature to ArrayProxy, as in

    # alternative signature 0 (current signature)
    def __init__(self, file_like, header, mmap=True):

    # alternative signature 1 (new signature)
    def __init__(self, file_like, shape, storage_dtype, offset=0, slope_inter=None, mmap=True)

Then we could use the second signature for the reshaped arrayproxy. The mmap argument is already keyword only, so this won't mess up calls with the current signature.

Then - it would be good to have more tests for new constructor and the reshape.

matthew-brett · 2017-03-25T10:13:43Z

nibabel/cifti2/parse_cifti2.py

@@ -147,7 +147,6 @@ def __init__(self, dataobj, affine, header=None,
        if self.cifti_img is None:
            raise ValueError('Nifti2 header does not contain a CIFTI2 '
                             'extension')
-        self.cifti_img.data = self.get_data()


Or could be self.cifti_img.dataobj = self.dataobj. This doesn't load the data, but keeps it as an array proxy.

I'm not sure the cifti_img attribute is doing any work at all. Even the check on the header is replicated here. I think it may be a refactoring hold-over that never got cleaned up. I'm pretty sure we could, without losing functionality, rewrite the entire class as:

class _Cifti2AsNiftiImage(Nifti2Image): header_class = _Cifti2AsNiftiHeader makeable = False

effigies · 2017-03-25T10:29:05Z

@demianw Great! Until this PR is finished, you can use c6192ad.

@matthew-brett I'll see what I can do. I've got a lot on my plate at the moment, so this may take a little bit of time to iterate through.

coveralls · 2017-03-25T10:29:16Z

Coverage decreased (-0.003%) to 96.154% when pulling c6192ad on effigies:ap_reshape into ca977ab on nipy:master.

effigies · 2017-03-25T17:28:03Z

@matthew-brett It looks like the FutureWarning has been on ArrayProxy.header since the 2.0 release, so I think it's reasonable to fast-track deprecation to 3.0.

How about this approach:

Add (and document) signature 1
Add .from_header classmethod
Update signature 0 docs to point to .from_header, raise a FutureWarning if signature 0 is used
- 3.0 (along with .header removal)
  - Move signature 0 to DeprecationWarning
  - Remove from docstring
- 4.0
  - Remove signature 0 support

effigies · 2017-03-25T18:15:49Z

No tests yet. Want to make sure I haven't broken old tests first.

matthew-brett · 2017-03-25T21:27:22Z

I like the deprecation scheme...

matthew-brett · 2017-03-28T14:19:11Z

nibabel/arrayproxy.py

+            The header should also have a 'copy' method.  This requirement will
+            go away when the deprecated 'header' property goes away.
+
+        (2) ArrayProxy(file_like, shape, storage_dtype,


Now I think of it, maybe the second argument can be either:

the header (as now)

a tuple of length 2 - 5 containing (shape, storage_dtype, [offset [slope, [inter]]]).

Then the signature can stay the same length, at the expense of slightly greater complexity of explanation / signature. What do you think?

I think that's a little less awkward. Do we still want to deprecate the header path, then?

No, I think we can leave the header path. I suppose we might rename the input argument, hoping that no-one out there is using ArrayProxy(header=something)...

effigies · 2017-03-28T19:22:03Z

@matthew-brett Ready for review.

matthew-brett

Thanks - a few questions / comments - but looking good.

matthew-brett · 2017-03-28T20:26:24Z

nibabel/arrayproxy.py

+            self._header = spec.copy()
+        elif 2 <= len(spec) <= 5:
+            optional = (0, 1., 0.)
+            par = spec + optional[len(spec) - 2:]


matthew-brett · 2017-03-28T20:34:53Z

nibabel/arrayproxy.py

        """ Initialize array proxy instance

        Parameters
        ----------
        file_like : object
            File-like object or filename. If file-like object, should implement
            at least ``read`` and ``seek``.
-        header : object
+        spec : object or tuple
+            Tuple must have length 2-5, with the following fields:


Suggest 'values' instead of 'fields' - 'fields' (to me) implies names, which might be confusing.

Anal, I know, but I prefer full-stops at the end of sentences in the docstring.

Used a period here. The rest seemed to be more fragments than sentences, but I can add periods there, if they're preferred, stylistically.

matthew-brett · 2017-03-28T20:35:48Z

nibabel/arrayproxy.py


    @property
+    @deprecate_with_version('ArrayProxy.header deprecated', '2.2', '3.0')


matthew-brett · 2017-03-28T20:36:24Z

nibabel/arrayproxy.py

@@ -162,6 +187,16 @@ def __getitem__(self, slicer):
        # Upcast as necessary for big slopes, intercepts
        return apply_read_scaling(raw_data, self._slope, self._inter)

+    def reshape(self, shape):


One-liner docstring?

Would be good to allow -1 as element in shape, for compatibility with numpy array reshape.

matthew-brett · 2017-03-28T20:38:00Z

nibabel/arrayproxy.py

+        if np.prod(shape) != size:
+            raise ValueError("cannot reshape array of size {:d} into shape "
+                             "{!s}".format(size, shape))
+        return ArrayProxy(file_like=self.file_like,


Maybe self.__class__(file_like=self.file_like ... to allow subclassing.

matthew-brett · 2017-03-28T20:41:04Z

nibabel/cifti2/parse_cifti2.py

    makeable = False
-    rw = True
-
-    def __init__(self, dataobj, affine, header=None,


I guess this is because the cifti_img attribute is not being used elsewhere?

Do we want to raise an error if no cifti extension found?

What is this subclass now for?

The point of the subclass is to be a Nifti2Image with this header, which has different constraints from a standard Nifti2Header.

matthew-brett · 2017-03-28T21:19:16Z

We might also want to set self._header = None for the tuple input argument case, and test that the header attribute correctly returns None.

effigies · 2017-03-28T21:51:55Z

I do set the default _header attribute to None: https://github.com/nipy/nibabel/pull/521/files#diff-644c66bb38fc2e28c018a29d87e96d36R69

Added a test.

matthew-brett

Nearly there - mainly some reshape stuff and tests.

matthew-brett · 2017-03-28T21:54:52Z

nibabel/tests/test_arrayproxy.py

+                         ):
+        assert_array_equal(getattr(ap_header, method)(*args),
+                           getattr(ap_tuple, method)(*args))
+    # Tuple-defined ArrayProxies have no header to store


This one raises a warning no? Need to protect with warnings context manager?

matthew-brett · 2017-03-28T21:58:45Z

nibabel/arrayproxy.py

+    def reshape(self, shape):
+        ''' Return an ArrayProxy with a new shape, without modifying data
+
+        ``array_proxy.reshape(shape)`` is equivalent to


I think these will give different results, no (I guess np.reshape will cast to an array, and it also allows the order keyword)? Maybe omit this sentence.

matthew-brett · 2017-03-28T22:00:40Z

nibabel/arrayproxy.py

+        # Calculate new shape if not fully specified
+        shape_arr = np.asarray(shape)
+        unknowns = shape_arr == -1
+        if len(unknowns) > 1:


This will be boolean array of same length as shape.

Maybe

from operator import mul from functools import reduce n_unknowns = len([e for e in shape if e == -1]) if n_unknowns > 1: raise ValueError("can only specify one unknown dimension") elif n_unknowns == 1: known_size = reduce(mul, shape, -1) unknown_size = size // known_size shape = [unknown_size if e == -1 else e for e in shape]

matthew-brett · 2017-03-28T22:15:21Z

nibabel/tests/test_arrayproxy.py

+    hdr = FunkyHeader(shape)
+    bio = BytesIO()
+    prox = ArrayProxy(bio, hdr)
+    assert_true(isinstance(prox.reshape((2, 3, 4)), ArrayProxy))


Need tests for -1 element, errors for incorrect size, to many -1s.

effigies · 2017-03-29T00:57:54Z

Thanks for the review. Kind of rushed that last push without testing first.

coveralls · 2017-03-29T01:16:25Z

Coverage increased (+0.005%) to 96.163% when pulling 8ad9c32 on effigies:ap_reshape into 8de91a1 on nipy:master.

coveralls · 2017-03-29T01:46:28Z

Coverage increased (+0.01%) to 96.172% when pulling 37405c3 on effigies:ap_reshape into 8de91a1 on nipy:master.

matthew-brett · 2017-03-29T06:53:10Z

Excellent - thanks. Good to go from my side. Any other comments?

effigies · 2017-03-29T10:21:20Z

I'm all set. Thanks for the review. I'll let you merge.

matthew-brett · 2017-03-29T12:03:53Z

An extra thought - what should we do for the other ArrayProxy API objects such as MincImageArray, EcatImageArray, PARRECArrayProxy ? Leave reshape to generate an AttributeError? Raise a NotImplementedError?

effigies · 2017-03-29T12:11:01Z

Given that the way to reshape has been with reshape_dataobj, I was intending to leave it be. If we do add reshape in whatever form to the other proxies, I think we'd need to switch that to a try/catch block, rather than a hasattr check, though.

matthew-brett · 2017-04-06T17:02:29Z

Thanks - let's put this in now and reflect at leisure about the other classes.

effigies added 3 commits March 24, 2017 13:12

Add ArrayProxy.reshape

4c4405b

Typo

1eb6fbe

Cifti2 arrayproxy tests should be negated or dropped...

8b41f55

RF: Do not automatically load data CIFTI data

c6192ad

matthew-brett reviewed Mar 25, 2017

View reviewed changes

matthew-brett reviewed Mar 28, 2017

View reviewed changes

effigies added 6 commits March 28, 2017 15:20

Severely simplify _Cifti2AsNiftiImage

d4e152a

Permit tuple ArrayProxy spec in place of header

49725c7

ArrayProxy.reshape generates headerless ArrayProxy

0c62d74

RF: Deprecate ArrayProxy.header to be removed in 3.0

1626cfb

TEST: Test tuple specs and reshaped APs

c6dfa6a

Update Changelog

ad35340

effigies force-pushed the ap_reshape branch from 10a3ab6 to ad35340 Compare March 28, 2017 19:21

matthew-brett reviewed Mar 28, 2017

View reviewed changes

effigies added 2 commits March 28, 2017 17:46

DOC: Update ArrayProxy docstrings

c6950f5

RF: Update ArrayProxy.reshape to take -1 entries

e35f327

matthew-brett reviewed Mar 28, 2017

View reviewed changes

effigies added 2 commits March 28, 2017 20:55

FIX: Correct reshape method

fbac7e4

TEST: Catch warning, test -1 reshaping

8ad9c32

effigies added 2 commits March 28, 2017 21:26

TEST: Check for illegal specs

f4f3894

TEST: Check legal but incompatible shapes

37405c3

effigies mentioned this pull request Mar 29, 2017

Transition FutureWarnings older than X versions to Deprecations? #524

Closed

matthew-brett merged commit 2584b23 into nipy:master Apr 6, 2017

effigies deleted the ap_reshape branch April 6, 2017 17:03


		@property
		@deprecate_with_version('ArrayProxy.header deprecated', '2.2', '3.0')

ENH: ArrayProxy reshape #521

ENH: ArrayProxy reshape #521

Conversation

effigies commented Mar 25, 2017

coveralls commented Mar 25, 2017

codecov-io commented Mar 25, 2017 • edited Loading

Codecov Report

demianw commented Mar 25, 2017 • edited Loading

matthew-brett commented Mar 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Mar 25, 2017

coveralls commented Mar 25, 2017

effigies commented Mar 25, 2017

effigies commented Mar 25, 2017

matthew-brett commented Mar 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Mar 28, 2017

matthew-brett 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-brett commented Mar 28, 2017

effigies commented Mar 28, 2017

matthew-brett 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

effigies commented Mar 29, 2017

coveralls commented Mar 29, 2017

coveralls commented Mar 29, 2017

matthew-brett commented Mar 29, 2017

effigies commented Mar 29, 2017

matthew-brett commented Mar 29, 2017

effigies commented Mar 29, 2017

matthew-brett commented Apr 6, 2017

codecov-io commented Mar 25, 2017 •

edited

Loading

demianw commented Mar 25, 2017 •

edited

Loading