Skip to content

More GIFTI fixes #403

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 6 commits into from
Feb 13, 2016
Merged

More GIFTI fixes #403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
255b751
deprecation tests should actually test for warnings.
Oct 17, 2015
b9fbbe0
data arrays created via constructor are malformed; create via from_ar…
Oct 17, 2015
5fef80d
Do not declare class variables as types; they mask failure to set ins…
Oct 17, 2015
23dc6c1
make num_dim a read-only property
Oct 17, 2015
78deca4
STY: fix up pep8 failures.
Feb 8, 2016
ae094ac
TST: add gifti string and MetaData tests.
Feb 12, 2016
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
39 changes: 30 additions & 9 deletions nibabel/gifti/gifti.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@ def print_summary(self):


class GiftiNVPairs(object):

"""
name = str
value = str

"""
Copy link
Member

Choose a reason for hiding this comment

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

For these, if possible, I'd rather see proper docstrings than simply commenting these out.

At simplest, a quick description and a type description, e.g.:

class GiftiNVPairs(object):
    """Name-value pair objects

    Instance variables
    ------------------
    name    : str
    value   : str
    """

Documenting instance variables doesn't seem to be standard nibabel practice, so I'm also open to just removing these. Still, seems a little bit of a shame to lose the type information altogether. Perhaps @matthew-brett has a feeling on best direction to go with these?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree, it's good to document these. I just looked that up in https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt#class-docstring

So I think this should be:

class GiftiNVPairs(object):
    """Name-value pair objects

    Attributes
    -------------
    name : str
        Maybe some text.
    value   : str
        Might be text here too.
    """

def __init__(self, name='', value=''):
self.name = name
self.value = value
Expand Down Expand Up @@ -104,6 +104,7 @@ def print_summary(self):


class GiftiLabel(xml.XmlSerializable):
"""
Copy link
Member

Choose a reason for hiding this comment

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

Ben - feel free to leave these, I'll submit another PR with docstring edits.

key = int
label = str
# rgba
Expand All @@ -114,6 +115,7 @@ class GiftiLabel(xml.XmlSerializable):
green = float
blue = float
alpha = float
"""

def __init__(self, key=0, label='', red=None, green=None, blue=None,
alpha=None):
Expand Down Expand Up @@ -157,9 +159,11 @@ def _arr2txt(arr, elem_fmt):


class GiftiCoordSystem(xml.XmlSerializable):
"""
dataspace = int
xformspace = int
xform = np.ndarray # 4x4 numpy array
"""

def __init__(self, dataspace=0, xformspace=0, xform=None):
self.dataspace = dataspace
Expand Down Expand Up @@ -224,12 +228,11 @@ def _data_tag_element(dataarray, encoding, datatype, ordering):


class GiftiDataArray(xml.XmlSerializable):

"""
# These are for documentation only; we don't use these class variables
intent = int
datatype = int
ind_ord = int
num_dim = int
dims = list
encoding = int
endian = int
Expand All @@ -238,19 +241,38 @@ class GiftiDataArray(xml.XmlSerializable):
data = np.ndarray
coordsys = GiftiCoordSystem
meta = GiftiMetaData
"""

def __init__(self, data=None):
def __init__(self, data=None,
encoding="GIFTI_ENCODING_B64GZ",
endian=sys.byteorder,
coordsys=None,
ordering="C",
meta=None):
Copy link
Member

Choose a reason for hiding this comment

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

None of these parameters are actually used except for data.

"""
Returns a shell object that cannot be saved.
"""
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have some docstring description for these guys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I have to say, I'm not clear enough on the GIFTI format to write these; I've simply been approaching GIFTI from the software engineering side.

Sounds like you're up for writing these, @matthew-brett ?

self.data = data
self.dims = []
self.meta = GiftiMetaData()
self.coordsys = GiftiCoordSystem()
self.meta = meta or GiftiMetaData()
self.coordsys = coordsys or GiftiCoordSystem()
self.ext_fname = ''
self.ext_offset = ''
self.intent = 0 # required attribute, NIFTI_INTENT_NONE
self.datatype = 0 # required attribute, void/none
# python/numpy default: column major order
self.ind_ord = array_index_order_codes.code[ordering]
self.encoding = encoding
self.endian = endian

@property
def num_dim(self):
return len(self.dims)

@classmethod
def from_array(klass,
darray,
intent,
intent="NIFTI_INTENT_NONE",
datatype=None,
encoding="GIFTI_ENCODING_B64GZ",
endian=sys.byteorder,
Expand Down Expand Up @@ -289,7 +311,6 @@ def from_array(klass,
if meta is None:
meta = {}
cda = klass(darray)
cda.num_dim = len(darray.shape)
cda.dims = list(darray.shape)
if datatype is None:
cda.datatype = data_type_codes.code[darray.dtype]
Copy link
Member

Choose a reason for hiding this comment

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

Given that you've reworked __init__, this function wouldn't take much work to clean up so that it ends with something like:

return klass(darray, encoding, endian, coordsys, ordering, meta)

(Again, sorry if I'm going deeper than you're trying to make this PR go.)

Expand Down
8 changes: 4 additions & 4 deletions nibabel/gifti/parse_gifti_fast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,14 @@ def StartElementHandler(self, name, attrs):
if "ArrayIndexingOrder" in attrs:
self.da.ind_ord = array_index_order_codes.code[
attrs["ArrayIndexingOrder"]]
if "Dimensionality" in attrs:
self.da.num_dim = int(attrs["Dimensionality"])
for i in range(self.da.num_dim):
num_dim = int(attrs.get("Dimensionality", 0))
for i in range(num_dim):
di = "Dim%s" % str(i)
if di in attrs:
self.da.dims.append(int(attrs[di]))
# dimensionality has to correspond to the number of DimX given
assert len(self.da.dims) == self.da.num_dim
# TODO (bcipolli): don't assert; raise parse warning, and recover.
assert len(self.da.dims) == num_dim
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to go ahead and raise the warning, or is that for a future iteration? (Given that functionality isn't changing, I'm okay with kicking this one down the road.)

if "Encoding" in attrs:
self.da.encoding = gifti_encoding_codes.code[attrs["Encoding"]]
if "Endian" in attrs:
Expand Down
37 changes: 28 additions & 9 deletions nibabel/gifti/tests/test_gifti.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import nibabel as nib
from nibabel.externals.six import string_types
from nibabel.gifti import (GiftiImage, GiftiDataArray, GiftiLabel,
GiftiLabelTable, GiftiMetaData)
GiftiLabelTable, GiftiMetaData, GiftiNVPairs)
from nibabel.gifti.gifti import data_tag
from nibabel.nifti1 import data_type_codes

Expand All @@ -34,29 +34,38 @@ def test_gifti_image():
gi = GiftiImage()
assert_equal(gi.numDA, 0)

da = GiftiDataArray(data='data')
# Test from numpy numeric array
Copy link
Member

Choose a reason for hiding this comment

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

It is worth also testing case of initializing from a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This could be tested separately, without saving, to validate that properties are being set.

data = np.random.random((5,))
da = GiftiDataArray.from_array(data)
gi.add_gifti_data_array(da)
assert_equal(gi.numDA, 1)
assert_equal(gi.darrays[0].data, 'data')
assert_array_equal(gi.darrays[0].data, data)

# Test removing
gi.remove_gifti_data_array(0)
assert_equal(gi.numDA, 0)

# Test from string
da = GiftiDataArray.from_array('zzzzz')
gi.add_gifti_data_array(da)
assert_equal(gi.numDA, 1)
assert_array_equal(gi.darrays[0].data, data)


# Remove from empty
gi = GiftiImage()
gi.remove_gifti_data_array_by_intent(0)
assert_equal(gi.numDA, 0)

# Remove one
gi = GiftiImage()
da = GiftiDataArray(data='data')
da = GiftiDataArray.from_array(np.zeros((5,)), intent=0)
gi.add_gifti_data_array(da)

gi.remove_gifti_data_array_by_intent(0)
assert_equal(gi.numDA, 1)
gi.remove_gifti_data_array_by_intent(3)
assert_equal(gi.numDA, 1, "data array should exist on 'missed' remove")

gi.darrays[0].intent = 0
gi.remove_gifti_data_array_by_intent(0)
gi.remove_gifti_data_array_by_intent(da.intent)
assert_equal(gi.numDA, 0)


Expand Down Expand Up @@ -97,13 +106,22 @@ def test_labeltable():
def test_metadata():
# Test deprecation
with clear_and_catch_warnings() as w:
warnings.filterwarnings('once', category=DeprecationWarning)
warnings.filterwarnings('always', category=DeprecationWarning)
assert_equal(len(GiftiDataArray().get_metadata()), 0)
assert_equal(len(w), 1)
Copy link
Member

Choose a reason for hiding this comment

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

Nice


# Test deprecation
with clear_and_catch_warnings() as w:
warnings.filterwarnings('once', category=DeprecationWarning)
assert_equal(len(GiftiMetaData().get_metadata()), 0)
assert_equal(len(w), 1)


def test_metadata():
nvpair = GiftiNVPairs('key', 'value')
da = GiftiMetaData(nvpair=nvpair)
assert_equal(da.data[0].name, 'key')
assert_equal(da.data[0].value, 'value')


def test_gifti_label_rgba():
Expand Down Expand Up @@ -133,6 +151,7 @@ def assign_rgba(gl, val):
with clear_and_catch_warnings() as w:
warnings.filterwarnings('once', category=DeprecationWarning)
assert_equal(kwargs['red'], gl3.get_rgba()[0])
assert_equal(len(w), 1)

# Test default value
gl4 = GiftiLabel()
Expand Down
2 changes: 1 addition & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@ deps =
deps =
[flake8]
max-line-length=100
ignore=D100,D101,D102,D103,D104,D105,D200,D201,D204,D205,D208,D210,D300,D301,D400,D401,D403,E266,E402,E731,F821,I100,I101,I201,N802,N803,N804,N806
ignore=D100,D101,D102,D103,D104,D105,D200,D201,D202,D204,D205,D208,D209,D210,D300,D301,D400,D401,D403,E266,E402,E731,F821,I100,I101,I201,N802,N803,N804,N806
exclude=*test*,*sphinx*,nibabel/externals/*,*/__init__.py