Merge branch 'rf/save_mmap' into 'master'

Rf/save mmap

See merge request fsl/fslpy!63

CHANGELOG.rst

@@ -2,6 +2,18 @@ This document contains the ``fslpy`` release history in reverse chronological
 order.
 
 
+1.10.2 (Friday September 7th 2018)
+----------------------------------
+
+
+Fixed
+^^^^^
+
+
+* The :meth:`.Image.save` method was not handling memory-mapped images
+  correctly.
+
+
 1.10.1 (Friday August 3rd 2018)
 -------------------------------
 
@@ -16,7 +28,7 @@ Changed
 Fixed
 ^^^^^
 
-* The .mod:`.FEATImage.getCOPE` method was returning PE images.
+* The :mod:`.FEATImage.getCOPE` method was returning PE images.
 
 
 1.10.0 (Wednesday July 18th 2018)

fsl/data/image.py

@@ -35,8 +35,10 @@ and file names:
 
 import                      os
 import os.path           as op
+import                      shutil
 import                      string
 import                      logging
+import                      tempfile
 
 import                      six
 import                      deprecation
@@ -1165,11 +1167,41 @@ class Image(Nifti):
         log.debug('Saving {} to {}'.format(self.name, filename))
 
         # If this Image is not managing its
-        # own file object, nibabel does all
-        # of the hard work.
-        if self.__fileobj is None:
+        # own file object, and the image is not
+        # memory-mapped, nibabel does all of
+        # the hard work.
+        newnibimage = False
+        ismmap      = isinstance(self[0, 0, :10], np.memmap)
+        if self.__fileobj is None and (not ismmap):
             nib.save(self.__nibImage, filename)
 
+        # If the image is memory-mapped, we need
+        # to close and re-open the image
+        elif self.__fileobj is None:
+
+            # We save the image out to a temp file,
+            # then close the old image, move the
+            # temp file to the real destination,
+            # then re-open the file.
+            newnibimage     = True
+            tmphd, tmpfname = tempfile.mkstemp(suffix=op.splitext(filename)[1])
+            os.close(tmphd)
+
+            try:
+                nib.save(self.__nibImage, tmpfname)
+
+                self.__nibImage = None
+                self.header     = None
+
+                shutil.copy(tmpfname, filename)
+
+                self.__nibImage = nib.load(filename)
+                self.header     = self.__nibImage.header
+
+            except Exception:
+                os.remove(tmpfname)
+                raise
+
         # Otherwise we've got our own file
         # handle to an IndexedGzipFile
         else:
@@ -1190,15 +1222,19 @@ class Image(Nifti):
             # compressed data. And then be able to
             # transfer the index generated from the
             # write to a new read-only file handle.
+            newnibimage = True
             nib.save(self.__nibImage, filename)
             self.__fileobj.close()
             self.__nibImage, self.__fileobj = loadIndexedImageFile(filename)
             self.header = self.__nibImage.header
 
-            # We have to create a new ImageWrapper
-            # instance too, as we have just destroyed
-            # the nibabel image we gave to the last
-            # one.
+        # If we've created a new nibabel image,
+        # we have to create a new ImageWrapper
+        # instance too, as we have just destroyed
+        # the nibabel image we gave to the last
+        # one.
+        if newnibimage:
+
             self.__imageWrapper.deregister(self.__lName)
             self.__imageWrapper = imagewrapper.ImageWrapper(
                 self.nibImage,

fsl/data/imagewrapper.py

@@ -825,6 +825,8 @@ def expectedShape(sliceobj, shape):
         if start is None: start = 0
         if stop  is None: stop  = shape[i]
 
+        stop = min(stop, shape[i])
+
         expShape.append(stop - start)
 
     return len(expShape), expShape

tests/test_image.py

@@ -994,29 +994,29 @@ def _test_Image_save(imgtype):
 
         make_image(filename, imgtype)
 
-        # Using mmap can cause a "Bus error"
-        # under docker. No idea why.
-        img = fslimage.Image(filename, mmap=False)
+        # Save to original location, and
+        # to a different location. And
+        # test both with and without mmap
+        targets = [None, filename, filename2]
+        mmaps   = [False, True]
 
-        randvoxes = randvoxes(5)
-        randvals  = [np.random.random() for i in range(5)]
+        for target, mmap in it.product(targets, mmaps):
 
-        for (x, y, z), v in zip(randvoxes, randvals):
-            img[x, y, z] = v
+            img = fslimage.Image(filename, mmap=mmap)
 
-        if imgtype > 0:
-            img.voxToWorldMat = xform
+            rvoxs = randvoxes(5)
+            rvals = [np.random.random() for i in range(5)]
 
-        # Save to original location, and
-        # to a different location
-        targets = [None, filename, filename2]
+            for (x, y, z), v in zip(rvoxs, rvals):
+                img[x, y, z] = v
 
-        for t in targets:
+            if imgtype > 0:
+                img.voxToWorldMat = xform
 
-            img.save(t)
+            img.save(target)
 
-            if t is None: expDataSource = filename
-            else:         expDataSource = t
+            if target is None: expDataSource = filename
+            else:              expDataSource = target
 
             assert img.saveState
             assert img.dataSource == expDataSource
@@ -1024,7 +1024,7 @@ def _test_Image_save(imgtype):
             if imgtype > 0:
                 assert np.all(np.isclose(img.voxToWorldMat, xform))
 
-            for (x, y, z), v in zip(randvoxes, randvals):
+            for (x, y, z), v in zip(rvoxs, rvals):
                 assert np.isclose(img[x, y, z], v)
 
             # Load the image back in
@@ -1036,7 +1036,7 @@ def _test_Image_save(imgtype):
             if imgtype > 0:
                 assert np.all(np.isclose(img.voxToWorldMat, xform))
 
-            for (x, y, z), v in zip(randvoxes, randvals):
+            for (x, y, z), v in zip(rvoxs, rvals):
                 assert np.isclose(img[x, y, z], v)
             img2 = None
 

tests/test_imagewrapper.py

@@ -257,6 +257,55 @@ def coverageDataRange(data, coverage, slices=None):
     return np.min(volmin), np.max(volmax)
 
 
+def test_expectedShape():
+
+    tests = [
+        ((slice(None), ), (10,),
+         (1, (10, ))),
+
+        ((slice(None), slice(None)),
+         (10, 10), (2, (10, 10))),
+
+        ((slice(None), slice(None), slice(None)),
+         (10, 10, 10), (3, (10, 10, 10))),
+
+        ((slice(None), slice(None), slice(None)),
+         (10, 10, 10), (3, (10, 10, 10))),
+
+        ((slice(None), slice(None), slice(None), slice(None)),
+         (10, 10, 10, 10), (4, (10, 10, 10, 10))),
+
+        ((1, slice(None), slice(None)),
+         (10, 10, 10), (2, (10, 10))),
+
+        ((slice(1, 3), slice(None), slice(None)),
+         (10, 10, 10), (3, (2, 10, 10))),
+
+        ((slice(None), 1, slice(None)),
+         (10, 10, 10), (2, (10, 10))),
+
+        ((slice(None), slice(1, 3), slice(None)),
+         (10, 10, 10), (3, (10, 2, 10))),
+
+        ((slice(None), slice(None), 1),
+         (10, 10, 10), (2, (10, 10))),
+
+        ((slice(None), slice(None), slice(1, 3), ),
+         (10, 10, 10), (3, (10, 10, 2))),
+
+        ((slice(None), slice(None), slice(1, 20), ),
+         (10, 10, 10), (3, (10, 10, 9))),
+    ]
+
+    for slc, shape, exp in tests:
+
+        explen, exp = exp
+        gotlen, got = imagewrap.expectedShape(slc, shape)
+
+        assert explen     == gotlen
+        assert tuple(exp) == tuple(got)
+
+
 def test_sliceObjToSliceTuple():
 
     func  = imagewrap.sliceObjToSliceTuple