Merge pull request #389 from matthew-brett/fix-freesurfer-load

MRG: fix for Freesurfer mmap bug

We were accidentally passing an ImageOpener instance as the file object to the array proxy, and Pyton 3.5 pointed this out by failing:

http://nipy.bic.berkeley.edu/builders/nibabel-bdist32-35/builds/9/steps/shell_6/logs/stdio

This branch fixes the Python 3.5 tests:

http://nipy.bic.berkeley.edu/builders/nibabel-bdist32-35/builds/11/steps/shell_6/logs/stdio

Author: matthew-brett

nibabel/fileholders.py

@@ -90,6 +90,12 @@ def same_file_as(self, other):
         return ((self.filename == other.filename) and
                 (self.fileobj == other.fileobj))
 
+    @property
+    def file_like(self):
+        """ Return ``self.fileobj`` if not None, otherwise ``self.filename``
+        """
+        return self.fileobj if self.fileobj is not None else self.filename
+
 
 def copy_file_map(file_map):
     ''' Copy mapping of fileholders given by `file_map`

nibabel/freesurfer/mghformat.py

@@ -496,11 +496,13 @@ def from_file_map(klass, file_map, mmap=True):
         '''
         if not mmap in (True, False, 'c', 'r'):
             raise ValueError("mmap should be one of {True, False, 'c', 'r'}")
-        mghf = file_map['image'].get_prepare_fileobj('rb')
+        img_fh = file_map['image']
+        mghf = img_fh.get_prepare_fileobj('rb')
         header = klass.header_class.from_fileobj(mghf)
         affine = header.get_affine()
         hdr_copy = header.copy()
-        data = klass.ImageArrayProxy(mghf, hdr_copy, mmap=mmap)
+        # Pass original image fileobj / filename to array proxy
+        data = klass.ImageArrayProxy(img_fh.file_like, hdr_copy, mmap=mmap)
         img = klass(data, affine, header, file_map=file_map)
         img._load_cache = {'header': hdr_copy,
                            'affine': affine.copy(),

nibabel/freesurfer/tests/test_mghformat.py

@@ -9,12 +9,15 @@
 '''Tests for mghformat reading writing'''
 
 import os
+import io
+import gzip
 
 import numpy as np
 
 from ...externals.six import BytesIO
-from .. import load, save, MGHImage
-from ..mghformat import MGHHeader, MGHError
+from .. import load, save
+from ...openers import ImageOpener
+from ..mghformat import MGHHeader, MGHError, MGHImage
 from ...tmpdirs import InTemporaryDirectory
 from ...fileholders import FileHolder
 
@@ -28,6 +31,8 @@
 
 from ...tests import test_spatialimages as tsi
 
+MGZ_FNAME = os.path.join(data_path, 'test.mgz')
+
 # sample voxel to ras matrix (mri_info --vox2ras)
 v2r = np.array([[1, 2, 3, -13], [2, 3, 1, -11.5],
                 [3, 1, 2, -11.5], [0, 0, 0, 1]], dtype=np.float32)
@@ -43,8 +48,7 @@ def test_read_mgh():
     # mri_volsynth --dim 3 4 5 2 --vol test.mgz
     # --cdircos 1 2 3 --rdircos 2 3 1 --sdircos 3 1 2
     # mri_volsynth is a FreeSurfer command
-    mgz_path = os.path.join(data_path, 'test.mgz')
-    mgz = load(mgz_path)
+    mgz = load(MGZ_FNAME)
 
     # header
     h = mgz.header
@@ -165,8 +169,7 @@ def test_header_updating():
     # Don't update the header information if the affine doesn't change.
     # Luckily the test.mgz dataset had a bad set of cosine vectors, so these
     # will be changed if the affine gets updated
-    mgz_path = os.path.join(data_path, 'test.mgz')
-    mgz = load(mgz_path)
+    mgz = load(MGZ_FNAME)
     hdr = mgz.header
     # Test against mri_info output
     exp_aff = np.loadtxt(BytesIO(b"""
@@ -230,6 +233,26 @@ def test_header_slope_inter():
     assert_equal(hdr.get_slope_inter(), (None, None))
 
 
+def test_mgh_load_fileobj():
+    # Checks the filename gets passed to array proxy
+    #
+    # This is a bit of an implementation detail, but the test is to make sure
+    # that we aren't passing ImageOpener objects to the array proxy, as these
+    # were confusing mmap on Python 3.  If there's some sensible reason not to
+    # pass the filename to the array proxy, please feel free to change this
+    # test.
+    img = MGHImage.load(MGZ_FNAME)
+    assert_equal(img.dataobj.file_like, MGZ_FNAME)
+    # Check fileobj also passed into dataobj
+    with ImageOpener(MGZ_FNAME) as fobj:
+        contents = fobj.read()
+    bio = io.BytesIO(contents)
+    fm = MGHImage.make_file_map(mapping=dict(image=bio))
+    img2 = MGHImage.from_file_map(fm)
+    assert_true(img2.dataobj.file_like is bio)
+    assert_array_equal(img.get_data(), img2.get_data())
+
+
 class TestMGHImage(tsi.TestSpatialImage, tsi.MmapImageMixin):
     """ Apply general image tests to MGHImage
     """

nibabel/tests/test_fileholders.py

@@ -50,4 +50,12 @@ def test_same_file_as():
     assert_true(fh3.same_file_as(fh4_again))
 
 
-
+def test_file_like():
+    # Test returning file object or filename
+    fh = FileHolder('a_fname')
+    assert_equal(fh.file_like, 'a_fname')
+    bio = BytesIO()
+    fh = FileHolder(fileobj=bio)
+    assert_true(fh.file_like is bio)
+    fh = FileHolder('a_fname', fileobj=bio)
+    assert_true(fh.file_like is bio)