Skip to content

More graceful handling of compressed file Opening #328

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.

Sign up for GitHub

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 7 commits into from
Aug 27, 2015
Merged

More graceful handling of compressed file Opening #328

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9875e1c
Allow classes to register extensions with opener functions; use this …
Jul 16, 2015
2f851a2
Use Opener (which contains .mgz) instead of BinOpener (which is no lo…
Aug 25, 2015
7410896
Move decorator to ImageOpener, use ImageOpener.
Aug 25, 2015
094f832
register_extension => register_ext_from_image
Aug 25, 2015
8da5d5c
Add simple tests for adding a new file association.
Aug 25, 2015
3d2777e
Add test for BinOpener deprecation.
Aug 27, 2015
f4c20f3
Tweak docstrings / parameter name.
Aug 27, 2015
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
4 changes: 2 additions & 2 deletions bin/nib-nifti-dx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ def main():
(opts, files) = parser.parse_args()

for fname in files:
with nib.volumeutils.BinOpener(fname) as fobj:
hdr = fobj.read(nib.nifti1.header_dtype.itemsize)
with nib.openers.ImageOpener(fname) as fobj:
hdr = fobj.read(nib.nifti1.header_dtype.itemsize)
result = nib.Nifti1Header.diagnose_binaryblock(hdr)
if len(result):
print('Picky header check output for "%s"\n' % fname)
Expand Down
7 changes: 4 additions & 3 deletions nibabel/arrayproxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@
"""
import warnings

from .volumeutils import BinOpener, array_from_file, apply_read_scaling
from .volumeutils import array_from_file, apply_read_scaling
from .fileslice import fileslice
from .keywordonly import kw_only_meth
from .openers import ImageOpener


class ArrayProxy(object):
Expand Down Expand Up @@ -130,7 +131,7 @@ def get_unscaled(self):

This is an optional part of the proxy API
'''
with BinOpener(self.file_like) as fileobj:
with ImageOpener(self.file_like) as fileobj:
raw_data = array_from_file(self._shape,
self._dtype,
fileobj,
Expand All @@ -145,7 +146,7 @@ def __array__(self):
return apply_read_scaling(raw_data, self._slope, self._inter)

def __getitem__(self, slicer):
with BinOpener(self.file_like) as fileobj:
with ImageOpener(self.file_like) as fileobj:
raw_data = fileslice(fileobj,
slicer,
self._shape,
Expand Down
6 changes: 3 additions & 3 deletions nibabel/benchmarks/bench_fileslice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import numpy as np

from io import BytesIO
from ..openers import Opener
from ..openers import ImageOpener
from ..fileslice import fileslice
from ..rstutils import rst_table
from ..tmpdirs import InTemporaryDirectory
Expand Down Expand Up @@ -47,10 +47,10 @@ def run_slices(file_like, repeat=3, offset=0, order='F'):
n_dim = len(SHAPE)
n_slicers = len(_slices_for_len(1))
times_arr = np.zeros((n_dim, n_slicers))
with Opener(file_like, 'wb') as fobj:
with ImageOpener(file_like, 'wb') as fobj:
fobj.write(b'\0' * offset)
fobj.write(arr.tostring(order=order))
with Opener(file_like, 'rb') as fobj:
with ImageOpener(file_like, 'rb') as fobj:
for i, L in enumerate(SHAPE):
for j, slicer in enumerate(_slices_for_len(L)):
sliceobj = [slice(None)] * n_dim
Expand Down
6 changes: 3 additions & 3 deletions nibabel/fileholders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

from copy import copy

from .volumeutils import BinOpener
from .openers import ImageOpener


class FileHolderError(Exception):
Expand Down Expand Up @@ -63,10 +63,10 @@ def get_prepare_fileobj(self, *args, **kwargs):
``self.pos``
'''
if self.fileobj is not None:
obj = BinOpener(self.fileobj) # for context manager
obj = ImageOpener(self.fileobj) # for context manager
obj.seek(self.pos)
elif self.filename is not None:
obj = BinOpener(self.filename, *args, **kwargs)
obj = ImageOpener(self.filename, *args, **kwargs)
if self.pos != 0:
obj.seek(self.pos)
else:
Expand Down
2 changes: 2 additions & 0 deletions nibabel/freesurfer/mghformat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from ..fileholders import FileHolder, copy_file_map
from ..arrayproxy import ArrayProxy
from ..keywordonly import kw_only_meth
from ..openers import ImageOpener

# mgh header
# See https://surfer.nmr.mgh.harvard.edu/fswiki/FsTutorial/MghFormat
Expand Down Expand Up @@ -453,6 +454,7 @@ def writeftr_to(self, fileobj):
fileobj.write(ftr_nd.tostring())


@ImageOpener.register_ext_from_image('.mgz', ImageOpener.gz_def)
class MGHImage(SpatialImage):
""" Class for MGH format image
"""
Expand Down
10 changes: 5 additions & 5 deletions nibabel/loadsave.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import numpy as np

from .filename_parser import types_filenames, splitext_addext
from .volumeutils import BinOpener, Opener
from .openers import ImageOpener
from .analyze import AnalyzeImage
from .spm2analyze import Spm2AnalyzeImage
from .nifti1 import Nifti1Image, Nifti1Pair, header_dtype as ni1_hdr_dtype
Expand Down Expand Up @@ -68,18 +68,18 @@ def guessed_image_type(filename):
elif lext == '.mnc':
# Look for HDF5 signature for MINC2
# https://www.hdfgroup.org/HDF5/doc/H5.format.html
with Opener(filename) as fobj:
with ImageOpener(filename) as fobj:
signature = fobj.read(4)
klass = Minc2Image if signature == b'\211HDF' else Minc1Image
elif lext == '.nii':
with BinOpener(filename) as fobj:
with ImageOpener(filename) as fobj:
binaryblock = fobj.read(348)
ft = which_analyze_type(binaryblock)
klass = Nifti2Image if ft == 'nifti2' else Nifti1Image
else: # might be nifti 1 or 2 pair or analyze of some sort
files_types = (('image', '.img'), ('header', '.hdr'))
filenames = types_filenames(filename, files_types)
with BinOpener(filenames['header']) as fobj:
with ImageOpener(filenames['header']) as fobj:
binaryblock = fobj.read(348)
ft = which_analyze_type(binaryblock)
if ft == 'nifti2':
Expand Down Expand Up @@ -208,7 +208,7 @@ def read_img_data(img, prefer='scaled'):
hdr.set_data_offset(dao.offset)
if default_scaling and (dao.slope, dao.inter) != (1, 0):
hdr.set_slope_inter(dao.slope, dao.inter)
with BinOpener(img_file_like) as fileobj:
with ImageOpener(img_file_like) as fileobj:
if prefer == 'scaled':
return hdr.data_from_fileobj(fileobj)
return hdr.raw_data_from_fileobj(fileobj)
Expand Down
4 changes: 2 additions & 2 deletions nibabel/nicom/dicomwrappers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

from . import csareader as csar
from .dwiparams import B2q, nearest_pos_semi_def, q2bg
from ..volumeutils import BinOpener
from ..openers import ImageOpener
from ..onetime import setattr_on_read as one_time


Expand Down Expand Up @@ -51,7 +51,7 @@ def wrapper_from_file(file_like, *args, **kwargs):
"""
import dicom

with BinOpener(file_like) as fobj:
with ImageOpener(file_like) as fobj:
dcm_data = dicom.read_file(fobj, *args, **kwargs)
return wrapper_from_data(dcm_data)

Expand Down
14 changes: 14 additions & 0 deletions nibabel/openers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,3 +146,17 @@ def __enter__(self):

def __exit__(self, exc_type, exc_val, exc_tb):
self.close_if_mine()


class ImageOpener(Opener):
""" Opener-type class passed to image classes to collect compressed extensions
"""
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add note here on the lines of

This class allows itself to have image extensions added to its class attributes,
via the `register_ex_from_images`.  The class can therefore change state
when image classes are defined.

@classmethod
def register_ext_from_image(opener_klass, ext, func):
"""Decorator"""
def decorate(klass):
assert ext not in opener_klass.compress_ext_map, \
"Cannot redefine extension-function mappings."
opener_klass.compress_ext_map[ext] = func
Copy link
Member

Choose a reason for hiding this comment

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

This will modify the Opener class globally no? So all 'Opener' instances will now effectively become BinOpener instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. The goal was to abstract Opener such that new opener methods don't require a new class, and that Opener could be used everywhere.

I went this way because Opener/BinOpener seemed to be used by functions expecting it to know about all file types, so making subclasses of Opener when needed, didn't seem like a good option in the end.

But perhaps I'm reading the code poorly... I was looking at bin/nib-nifti-dx, nibabel/arrayproxy.py, and similar.

Copy link
Member

Choose a reason for hiding this comment

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

Forgive my anxiety here, but this decorator, though clever, makes me anxious. It's unexpected to me that a class decorator should alter the global state - in this case of attributes in another class. It makes it harder to reason about Opener. For example, if someone imports Opener from outside nibabel, they will have different file extensions depending on whether freesurfer stuff got imported or no.

Can you see any way round that?

Copy link
Member

Choose a reason for hiding this comment

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

For example, we only need the .mgz compressed extension to be in play for freesurfer files. So maybe we can get away with having a MgzOpener class that is the same as current BinOpener, and putting the class (Opener) into ArrayProxy and FileHolder classes as class attributes. Then the MGH format can subclass these two objects to add the MgzOpener class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if someone imports Opener from outside nibabel, they will have different file extensions depending on whether freesurfer stuff got imported or no.

I believe that this depends what you mean "outside nibabel". If it imports nibabel, then all known image classes are imported, and Opener will have all image extensions. If Opener is imported as a source file / outside of the nibabel package (for example, as a source file to use the base class), then it will not have all extensions. This makes sense to me.

But I feel I'm probably not understanding your concern properly.

For example, we only need the .mgz compressed extension to be in play for freesurfer files. So maybe we can get away with having a MgzOpener class that is the same as current BinOpener, and putting the class (Opener) into ArrayProxy and FileHolder classes as class attributes. Then the MGH format can subclass these two objects to add the MgzOpener class?

I don't like this because each time a new extension is added, a new Opener class needs to be defined, and the code in non-local places needs to change. I find it much cleaner and clearer that everything about an image type should be defined in an image class--how to construct it, read it (Openers), and how to write it. The design you suggest requires adding information outside of that image class.

return klass
return decorate
9 changes: 5 additions & 4 deletions nibabel/parrec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,11 @@
from .keywordonly import kw_only_meth
from .spatialimages import SpatialImage, Header
from .eulerangles import euler2mat
from .volumeutils import Recoder, array_from_file, BinOpener
from .volumeutils import Recoder, array_from_file
from .affines import from_matvec, dot_reduce, apply_affine
from .nifti1 import unit_codes
from .fileslice import fileslice, strided_scalar
from .openers import ImageOpener

# PSL to RAS affine
PSL_TO_RAS = np.array([[0, 0, -1, 0], # L -> R
Expand Down Expand Up @@ -581,13 +582,13 @@ def is_proxy(self):
return True

def get_unscaled(self):
with BinOpener(self.file_like) as fileobj:
with ImageOpener(self.file_like) as fileobj:
return _data_from_rec(fileobj, self._rec_shape, self._dtype,
self._slice_indices, self._shape,
mmap=self._mmap)

def __array__(self):
with BinOpener(self.file_like) as fileobj:
with ImageOpener(self.file_like) as fileobj:
return _data_from_rec(fileobj,
self._rec_shape,
self._dtype,
Expand All @@ -603,7 +604,7 @@ def __getitem__(self, slicer):
return np.asanyarray(self)[slicer]
# Slices all sequential from zero, can use fileslice
# This gives more efficient volume by volume loading, for example
with BinOpener(self.file_like) as fileobj:
with ImageOpener(self.file_like) as fileobj:
raw_data = fileslice(fileobj, slicer, self._shape, self._dtype, 0,
'F')
# Broadcast scaling to shape of original data
Expand Down
4 changes: 2 additions & 2 deletions nibabel/tests/test_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import numpy as np

from ..openers import Opener
from ..openers import ImageOpener
from ..tmpdirs import InTemporaryDirectory
from ..optpkg import optional_package
_, have_scipy, _ = optional_package('scipy.io')
Expand Down Expand Up @@ -49,7 +49,7 @@ def bz2_mio_error():
import scipy.io

with InTemporaryDirectory():
with Opener('test.mat.bz2', 'wb') as fobj:
with ImageOpener('test.mat.bz2', 'wb') as fobj:
try:
scipy.io.savemat(fobj, {'a': 1}, format='4')
except ValueError:
Expand Down
37 changes: 35 additions & 2 deletions nibabel/tests/test_openers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@

from ..tmpdirs import InTemporaryDirectory

from ..openers import Opener
from ..openers import Opener, ImageOpener

from nose.case import Test
from nose.tools import (assert_true, assert_false, assert_equal,
assert_not_equal, assert_raises)

Expand Down Expand Up @@ -57,7 +58,6 @@ def test_Opener():
# mode is gently ignored
fobj = Opener(obj, mode='r')


def test_Opener_various():
# Check we can do all sorts of files here
message = b"Oh what a giveaway"
Expand Down Expand Up @@ -86,6 +86,39 @@ def test_Opener_various():
assert_not_equal(fobj.fileno(), 0)


class TestImageOpener:
def setUp(self):
self.compress_ext_map = ImageOpener.compress_ext_map.copy()

def teardown(self):
ImageOpener.compress_ext_map = self.compress_ext_map

def test_vanilla(self):
# Test that ImageOpener does add '.mgz' as gzipped file type
with InTemporaryDirectory():
with ImageOpener('test.gz', 'w') as fobj:
assert_true(hasattr(fobj.fobj, 'compress'))
with ImageOpener('test.mgz', 'w') as fobj:
assert_true(hasattr(fobj.fobj, 'compress'))

def test_new_association(self):
def file_opener(fileish, mode):
return open(fileish, mode)

# Add the association
n_associations = len(ImageOpener.compress_ext_map)
dec = ImageOpener.register_ext_from_image('.foo',
(file_opener, ('mode',)))
dec(self.__class__)
assert_equal(n_associations + 1, len(ImageOpener.compress_ext_map))
assert_true('.foo' in ImageOpener.compress_ext_map)

with InTemporaryDirectory():
with ImageOpener('test.foo', 'w'):
pass
assert_true(os.path.exists('test.foo'))


def test_file_like_wrapper():
# Test wrapper using BytesIO (full API)
message = b"History of the nude in"
Expand Down
4 changes: 2 additions & 2 deletions nibabel/tests/test_parrec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from .. import parrec
from ..parrec import (parse_PAR_header, PARRECHeader, PARRECError, vol_numbers,
vol_is_full, PARRECImage, PARRECArrayProxy, exts2pars)
from ..openers import Opener
from ..openers import ImageOpener
from ..fileholders import FileHolder
from ..volumeutils import array_from_file

Expand All @@ -32,7 +32,7 @@
DATA_PATH = pjoin(dirname(__file__), 'data')
EG_PAR = pjoin(DATA_PATH, 'phantom_EPI_asc_CLEAR_2_1.PAR')
EG_REC = pjoin(DATA_PATH, 'phantom_EPI_asc_CLEAR_2_1.REC')
with Opener(EG_PAR, 'rt') as _fobj:
with ImageOpener(EG_PAR, 'rt') as _fobj:
HDR_INFO, HDR_DEFS = parse_PAR_header(_fobj)
# Fake truncated
TRUNC_PAR = pjoin(DATA_PATH, 'phantom_truncated.PAR')
Expand Down
26 changes: 8 additions & 18 deletions nibabel/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,11 @@
import numpy as np

from ..tmpdirs import InTemporaryDirectory

from ..openers import ImageOpener
from ..volumeutils import (array_from_file,
_is_compressed_fobj,
array_to_file,
allopen, # for backwards compatibility
BinOpener,
allopen, # for backwards compatibility
fname_ext_ul_case,
calculate_scale,
can_cast,
Expand Down Expand Up @@ -928,7 +927,7 @@ def test_seek_tell():
st = functools.partial(seek_tell, write0=write0)
bio.seek(0)
# First write the file
with BinOpener(in_file, 'wb') as fobj:
with ImageOpener(in_file, 'wb') as fobj:
assert_equal(fobj.tell(), 0)
# already at position - OK
st(fobj, 0)
Expand All @@ -949,7 +948,7 @@ def test_seek_tell():
fobj.write(b'\x02' * tail)
bio.seek(0)
# Now read back the file testing seek_tell in reading mode
with BinOpener(in_file, 'rb') as fobj:
with ImageOpener(in_file, 'rb') as fobj:
assert_equal(fobj.tell(), 0)
st(fobj, 0)
assert_equal(fobj.tell(), 0)
Expand All @@ -961,22 +960,22 @@ def test_seek_tell():
st(fobj, 0)
bio.seek(0)
# Check we have the expected written output
with BinOpener(in_file, 'rb') as fobj:
with ImageOpener(in_file, 'rb') as fobj:
assert_equal(fobj.read(),
b'\x01' * start + b'\x00' * diff + b'\x02' * tail)
for in_file in ('test2.gz', 'test2.bz2'):
# Check failure of write seek backwards
with BinOpener(in_file, 'wb') as fobj:
with ImageOpener(in_file, 'wb') as fobj:
fobj.write(b'g' * 10)
assert_equal(fobj.tell(), 10)
seek_tell(fobj, 10)
assert_equal(fobj.tell(), 10)
assert_raises(IOError, seek_tell, fobj, 5)
# Make sure read seeks don't affect file
with BinOpener(in_file, 'rb') as fobj:
with ImageOpener(in_file, 'rb') as fobj:
seek_tell(fobj, 10)
seek_tell(fobj, 0)
with BinOpener(in_file, 'rb') as fobj:
with ImageOpener(in_file, 'rb') as fobj:
assert_equal(fobj.read(), b'g' * 10)


Expand Down Expand Up @@ -1004,15 +1003,6 @@ def seek(self, *args):
assert_equal(bio.getvalue(), ZEROB * 20)


def test_BinOpener():
# Test that BinOpener does add '.mgz' as gzipped file type
with InTemporaryDirectory():
with BinOpener('test.gz', 'w') as fobj:
assert_true(hasattr(fobj.fobj, 'compress'))
with BinOpener('test.mgz', 'w') as fobj:
assert_true(hasattr(fobj.fobj, 'compress'))


def test_fname_ext_ul_case():
# Get filename ignoring the case of the filename extension
with InTemporaryDirectory():
Expand Down
Loading