Skip to content

ENH: Define Cifti2 Axes describing the rows/columns of the Cifti2 data #641

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.

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 60 commits into from
Mar 31, 2019
Merged
Changes from 1 commit
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
87c7dc1
ENH: Define Cifti2 Axes describing the rows/columns of the Cifti2 data
MichielCottaar Jun 25, 2018
0e7566a
Clarified the test filenames
MichielCottaar Jun 25, 2018
671c196
BUG: removed failing doctest
MichielCottaar Jun 25, 2018
1dd059d
DOC: explained that Axis is an abstract class
MichielCottaar Jan 28, 2019
8660afd
RF: removed the typed array underlying all axes
MichielCottaar Mar 14, 2019
c2274c5
RF: increased consistency of methods in the Axis objects
MichielCottaar Mar 14, 2019
52aff8a
ENH: made it easier to create Label axis from constructor directly
MichielCottaar Mar 14, 2019
dad74ee
REF: made flake8 happy
MichielCottaar Mar 14, 2019
a0e8ff3
BF: fixed abstract class for python2
MichielCottaar Mar 14, 2019
420dacb
BF: allow any integer type, not just int
MichielCottaar Mar 15, 2019
29448f1
DOC: fixed many issues with the documentation
MichielCottaar Mar 15, 2019
5684268
RF: made flake8 happy again
MichielCottaar Mar 15, 2019
fbd28dc
Apply suggestions from code review
effigies Mar 19, 2019
c33e0d1
Added other reviewer suggestions
MichielCottaar Mar 19, 2019
bc2064e
RF: remove spurious ', ' from method definition
MichielCottaar Mar 19, 2019
4c0f389
Replaced asarray with asanyarray
MichielCottaar Mar 19, 2019
72c089a
reverse guard for incorrect type when concatenating parcels
MichielCottaar Mar 19, 2019
e8bcaba
Replaces type(self) with self.__class__
MichielCottaar Mar 19, 2019
ac2618b
Tests many more fail conditions and edge cases
MichielCottaar Mar 19, 2019
e00143b
MNT: Bump minimum numpy version to 1.8
effigies Mar 20, 2019
436231c
Apply suggestions from code review
effigies Mar 22, 2019
d1bc7fe
BF: only set idx_start to size - 1 for negative step
MichielCottaar Mar 22, 2019
949da0e
TEST: add tests for Axis __eq__ methods
MichielCottaar Mar 22, 2019
ac0258c
DOC: removed Series attribute list as it is already listed in the __i…
MichielCottaar Mar 22, 2019
6726b00
RF: removed separate `extend` method
MichielCottaar Mar 22, 2019
790aba4
BF: report original index
MichielCottaar Mar 22, 2019
e7302d6
RF: removed spurious '+' when concatenating literal strings
MichielCottaar Mar 22, 2019
b2c674f
Increased test coverage
MichielCottaar Mar 22, 2019
459fa88
RF: replaced last '.all()' with `array_equal`
MichielCottaar Mar 22, 2019
70588f2
Merge branch 'master' into enh/cifti2_axes
MichielCottaar Mar 22, 2019
6b55e11
RF: made flake8 happy again
MichielCottaar Mar 22, 2019
2ae25b4
Merge pull request #2 from effigies/enh/cifti2_axes
MichielCottaar Mar 22, 2019
e4bc7b0
BF: don't use np.full to create string array
MichielCottaar Mar 22, 2019
a5f88c2
RF: set surface BrainModel default structure to Other
MichielCottaar Mar 24, 2019
a31af61
RF: adjusted some type desciptions
MichielCottaar Mar 24, 2019
0e0b7f2
DOC: Added tutorial to docs
MichielCottaar Mar 24, 2019
594b22f
BUG: skip doctests in example code
MichielCottaar Mar 24, 2019
29a5287
BF: reduces line length in example code
MichielCottaar Mar 24, 2019
109fa92
RF: changes from @demianw not involving name changes
MichielCottaar Mar 26, 2019
f7c47bb
RF: rename from_mapping to from_index_mapping
MichielCottaar Mar 26, 2019
b88d516
RF: return CIFTI_MODEL_TYPE to distinguish surface and voxels
MichielCottaar Mar 26, 2019
fd8593e
RF: renamed is_surface to surface_mask and added volume_mask
MichielCottaar Mar 26, 2019
cdf57db
STY: fixed indentation
MichielCottaar Mar 26, 2019
b51d5f1
DOC: consistently use CIFTI-2 instead of CIfTI2 or CIFTI2
MichielCottaar Mar 27, 2019
532bed9
RF: appended Axis to the different axes classes
MichielCottaar Mar 27, 2019
cffb8c0
DOC: test creation of `bm_thal` and `bm_cortex`
MichielCottaar Mar 27, 2019
9b2276d
DOC: got rid of most of the :class:`Axis` in tutorial
MichielCottaar Mar 27, 2019
110334b
Update nibabel/cifti2/cifti2.py
effigies Mar 27, 2019
063047f
Apply suggestions from code review
effigies Mar 27, 2019
cefb8c6
RF: replace ~surface_mask with volume_mask
MichielCottaar Mar 27, 2019
0f0e1f7
DOC: added list of concrete classes to Axis object
MichielCottaar Mar 27, 2019
0270ad9
BF: add name to return, so that link works in html
MichielCottaar Mar 27, 2019
ed76418
RF: fix line length
MichielCottaar Mar 27, 2019
d825a3c
DOC: make format in list of axes more consistent
MichielCottaar Mar 27, 2019
1bc459e
DOCTEST: Drop doctest SKIP directives
effigies Mar 27, 2019
04a4b45
FIX: Use other.volume_mask to index other.voxel
effigies Mar 27, 2019
4aa3609
DOC: Update docstrings with a few more links and array_like
effigies Mar 27, 2019
97e16d3
Merge pull request #3 from effigies/enh/cifti2_axes
MichielCottaar Mar 27, 2019
36c162d
BF: doctest fixes for tutorial
MichielCottaar Mar 28, 2019
0927424
BF: fixed doctest for python 2.7
MichielCottaar Mar 29, 2019
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
Prev Previous commit
Next Next commit
REF: made flake8 happy
  • Loading branch information
MichielCottaar committed Mar 14, 2019
commit dad74eee9c0fb3d5dcddc89d1d9312d684f9d06e
119 changes: 78 additions & 41 deletions nibabel/cifti2/cifti2_axes.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,12 +123,14 @@ class BrainModel(Axis):
(N, ) array with the vertex indices
"""

def __init__(self, name, voxel=None, vertex=None, affine=None, volume_shape=None, nvertices=None):
def __init__(self, name, voxel=None, vertex=None, affine=None,
volume_shape=None, nvertices=None):
"""
Creates a new BrainModel axis defining the vertices and voxels represented by each row/column
Creates a BrainModel axis defining the vertices and voxels represented by each row/column

A more convenient way to create BrainModel axes is provided by the factory methods:
- `from_mask`: creates a surface or volumetric BrainModel axis from respectively 1D and 3D masks
- `from_mask`: creates surface or volumetric BrainModel axis from respectively
1D or 3D masks
- `from_surface`: creates a volumetric BrainModel axis

The resulting BrainModel axes can be concatenated by adding them together.
Expand All @@ -138,13 +140,16 @@ def __init__(self, name, voxel=None, vertex=None, affine=None, volume_shape=None
name : str or np.ndarray
brain structure name or (N, ) array with the brain structure names
voxel : np.ndarray
(N, 3) array with the voxel indices (can be omitted for CIFTI files only covering the surface)
(N, 3) array with the voxel indices (can be omitted for CIFTI files only
covering the surface)
vertex : np.ndarray
(N, ) array with the vertex indices (can be omitted for volumetric CIFTI files)
affine : np.ndarray
(4, 4) array mapping voxel indices to mm space (not needed for CIFTI files only covering the surface)
(4, 4) array mapping voxel indices to mm space (not needed for CIFTI files only
covering the surface)
volume_shape : Tuple[int, int, int]
shape of the volume in which the voxels were defined (not needed for CIFTI files only covering the surface)
shape of the volume in which the voxels were defined (not needed for CIFTI files only
covering the surface)
nvertices : dict[String -> int]
maps names of surface elements to integers (not needed for volumetric CIFTI files)
"""
Expand All @@ -157,7 +162,10 @@ def __init__(self, name, voxel=None, vertex=None, affine=None, volume_shape=None
nelements = len(voxel)
self.voxel = np.asarray(voxel, dtype=int)

self.vertex = -np.ones(nelements, dtype=int) if vertex is None else np.asarray(vertex, dtype=int)
if vertex is None:
self.vertex = -np.ones(nelements, dtype=int)
else:
self.vertex = np.asarray(vertex, dtype=int)

if isinstance(name, string_types):
name = [self.to_cifti_brain_structure_name(name)] * self.vertex.size
Expand Down Expand Up @@ -280,7 +288,10 @@ def from_mapping(cls, mim):
else:
if shape != mim.volume.volume_dimensions:
raise ValueError("All volume masks should be defined in the same volume")
if (affine != mim.volume.transformation_matrix_voxel_indices_ijk_to_xyz.matrix).any():
if (
affine !=
mim.volume.transformation_matrix_voxel_indices_ijk_to_xyz.matrix
).any():
raise ValueError("All volume masks should have the same affine")
return cls(name, voxel, vertex, affine, shape, nvertices)

Expand Down Expand Up @@ -309,11 +320,13 @@ def to_mapping(self, dim):
vertices = None
nvertex = None
if mim.volume is None:
affine = cifti2.Cifti2TransformationMatrixVoxelIndicesIJKtoXYZ(-3, matrix=self.affine)
affine = cifti2.Cifti2TransformationMatrixVoxelIndicesIJKtoXYZ(-3, self.affine)
mim.volume = cifti2.Cifti2Volume(self.volume_shape, affine)
cifti_bm = cifti2.Cifti2BrainModel(to_slice.start, len(bm),
'CIFTI_MODEL_TYPE_SURFACE' if is_surface else 'CIFTI_MODEL_TYPE_VOXELS',
name, nvertex, voxels, vertices)
cifti_bm = cifti2.Cifti2BrainModel(
to_slice.start, len(bm),
'CIFTI_MODEL_TYPE_SURFACE' if is_surface else 'CIFTI_MODEL_TYPE_VOXELS',
name, nvertex, voxels, vertices
)
mim.append(cifti_bm)
return mim

Expand Down Expand Up @@ -401,8 +414,8 @@ def to_cifti_brain_structure_name(name):
else:
proposed_name = 'CIFTI_STRUCTURE_%s_%s' % (structure.upper(), orientation.upper())
if proposed_name not in cifti2.CIFTI_BRAIN_STRUCTURES:
raise ValueError('%s was interpreted as %s, which is not a valid CIFTI brain structure' %
(name, proposed_name))
raise ValueError('%s was interpreted as %s, which is not a valid CIFTI brain structure'
% (name, proposed_name))
return proposed_name

@property
Expand Down Expand Up @@ -460,7 +473,7 @@ def name(self, ):
def name(self, values):
self._name = np.array([self.to_cifti_brain_structure_name(name) for name in values])

def __len__(self ):
def __len__(self):
return self.name.size

def __eq__(self, other):
Expand Down Expand Up @@ -496,14 +509,17 @@ def __add__(self, other):
affine, shape = other.affine, other.volume_shape
else:
affine, shape = self.affine, self.volume_shape
if other.affine is not None and ((other.affine != affine).all() or
other.volume_shape != shape):
raise ValueError("Trying to concatenate two BrainModels defined in a different brain volume")
if other.affine is not None and (
(other.affine != affine).all() or
other.volume_shape != shape
):
raise ValueError("Trying to concatenate two BrainModels defined " +
"in a different brain volume")
Copy link
Member

Choose a reason for hiding this comment

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

Can simplify these 10 lines:

affine, shape = other.affine, other.volume_shape
if self.affine is not None and (
        not np.allclose(affine, self.affine) or
        self.volume_shape != shape):
    raise ValueError(...)

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 the other BrainModel has an affine or volume_shape of None (which is expected if it does not contain any voxels), this change will lead to an error.

The existing code ensures two things:

  1. If only one of the BrainModels has volumetric data (i.e. the affine is not set to None), the local variables affine and volume_shape will be set to that BrainModel.
  2. If both BrainModels have volumetric data, they are checked for consistency of the affine and volume_shape.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. Sorry about that.

nvertices = dict(self.nvertices)
for name, value in other.nvertices.items():
if name in nvertices.keys() and nvertices[name] != value:
raise ValueError("Trying to concatenate two BrainModels with inconsistent number of vertices for %s"
% name)
raise ValueError("Trying to concatenate two BrainModels with inconsistent " +
"number of vertices for %s" % name)
nvertices[name] = value
return type(self)(
np.append(self.name, other.name),
Expand Down Expand Up @@ -570,7 +586,7 @@ class Parcels(Axis):

def __init__(self, name, voxels, vertices, affine=None, volume_shape=None, nvertices=None):
"""
Creates a new BrainModel axis defining the vertices and voxels represented by each row/column
Creates a Parcels axis defining the vertices and voxels represented by each row/column

Parameters
----------
Expand All @@ -581,9 +597,11 @@ def __init__(self, name, voxels, vertices, affine=None, volume_shape=None, nvert
vertices : np.ndarray
(N, ) object array each containing a sequence of vertices
affine : np.ndarray
(4, 4) array mapping voxel indices to mm space (not needed for CIFTI files only covering the surface)
(4, 4) array mapping voxel indices to mm space (not needed for CIFTI files only
covering the surface)
volume_shape : Tuple[int, int, int]
shape of the volume in which the voxels were defined (not needed for CIFTI files only covering the surface)
shape of the volume in which the voxels were defined (not needed for CIFTI files only
covering the surface)
nvertices : dict[String -> int]
maps names of surface elements to integers (not needed for volumetric CIFTI files)
"""
Expand Down Expand Up @@ -632,15 +650,16 @@ def from_brain_models(cls, named_brain_models):
volume_shape = bm.volume_shape
else:
if (affine != bm.affine).any() or (volume_shape != bm.volume_shape):
raise ValueError(
"Can not combine brain models defined in different volumes into a single Parcel axis")
raise ValueError("Can not combine brain models defined in different " +
"volumes into a single Parcel axis")
all_voxels.append(voxels)

vertices = {}
for name, _, bm_part in bm.iter_structures():
if name in bm.nvertices.keys():
if name in nvertices.keys() and nvertices[name] != bm.nvertices[name]:
raise ValueError("Got multiple conflicting number of vertices for surface structure %s" % name)
raise ValueError("Got multiple conflicting number of " +
"vertices for surface structure %s" % name)
nvertices[name] = bm.nvertices[name]
vertices[name] = bm_part.vertex
all_vertices.append(vertices)
Expand All @@ -665,7 +684,9 @@ def from_mapping(cls, mim):
all_vertices = np.zeros(nparcels, dtype='object')

volume_shape = None if mim.volume is None else mim.volume.volume_dimensions
affine = None if mim.volume is None else mim.volume.transformation_matrix_voxel_indices_ijk_to_xyz.matrix
affine = None
if mim.volume is not None:
affine = mim.volume.transformation_matrix_voxel_indices_ijk_to_xyz.matrix
nvertices = {}
for surface in mim.surfaces:
nvertices[surface.brain_structure] = surface.surface_number_of_vertices
Expand All @@ -679,7 +700,8 @@ def from_mapping(cls, mim):
name = vertex.brain_structure
vertices[vertex.brain_structure] = np.array(vertex)
if name not in nvertices.keys():
raise ValueError("Number of vertices for surface structure %s not defined" % name)
raise ValueError("Number of vertices for surface structure %s not defined" %
name)
all_voxels[idx_parcel] = voxels
all_vertices[idx_parcel] = vertices
all_names.append(parcel.name)
Expand Down Expand Up @@ -749,9 +771,11 @@ def __eq__(self, other):
any((vox1 != vox2).any() for vox1, vox2 in zip(self.voxels, other.voxels))):
return False
if self.affine is not None:
if ( other.affine is None or
if (
other.affine is None or
abs(self.affine - other.affine).max() > 1e-8 or
self.volume_shape != other.volume_shape):
self.volume_shape != other.volume_shape
):
return False
elif other.affine is not None:
return False
Expand Down Expand Up @@ -783,11 +807,13 @@ def __add__(self, other):
affine, shape = self.affine, self.volume_shape
if other.affine is not None and ((other.affine != affine).all() or
other.volume_shape != shape):
raise ValueError("Trying to concatenate two Parcels defined in a different brain volume")
raise ValueError("Trying to concatenate two Parcels defined " +
"in a different brain volume")
nvertices = dict(self.nvertices)
for name, value in other.nvertices.items():
if name in nvertices.keys() and nvertices[name] != value:
raise ValueError("Trying to concatenate two Parcels with inconsistent number of vertices for %s"
raise ValueError("Trying to concatenate two Parcels with inconsistent " +
"number of vertices for %s"
% name)
nvertices[name] = value
return type(self)(
Expand Down Expand Up @@ -838,7 +864,8 @@ def get_element(self, index):

class Scalar(Axis):
"""
Along this axis of the CIFTI vector/matrix each row/column has been given a unique name and optionally metadata
Along this axis of the CIFTI vector/matrix each row/column has been given
a unique name and optionally metadata
"""

def __init__(self, name, meta=None):
Expand All @@ -850,7 +877,8 @@ def __init__(self, name, meta=None):
name : np.ndarray
(N, ) string array with the parcel names
meta : np.ndarray
(N, ) object array with a dictionary of metadata for each row/column. Defaults to empty dictionary
(N, ) object array with a dictionary of metadata for each row/column.
Defaults to empty dictionary
"""
self.name = np.asarray(name, dtype='U')
if meta is None:
Expand Down Expand Up @@ -977,8 +1005,9 @@ def __init__(self, name, label, meta=None):
name : np.ndarray
(N, ) string array with the parcel names
label : np.ndarray
single dictionary or (N, ) object array with dictionaries mapping from integers to (name, (R, G, B, A)),
where name is a string and R, G, B, and A are floats between 0 and 1 giving the colour and alpha
single dictionary or (N, ) object array with dictionaries mapping
from integers to (name, (R, G, B, A)), where name is a string and R, G, B, and A are
floats between 0 and 1 giving the colour and alpha (i.e., transparency)
meta : np.ndarray
(N, ) object array with a dictionary of metadata for each row/column
"""
Expand Down Expand Up @@ -1056,7 +1085,11 @@ def __eq__(self, other):
"""
if not isinstance(other, Label) or self.size != other.size:
return False
return (self.name == other.name).all() and (self.meta == other.meta).all() and (self.label == other.label).all()
return (
(self.name == other.name).all() and
(self.meta == other.meta).all() and
(self.label == other.label).all()
)

def __add__(self, other):
"""
Expand Down Expand Up @@ -1191,7 +1224,8 @@ def unit(self, ):
@unit.setter
def unit(self, value):
if value.upper() not in ("SECOND", "HERTZ", "METER", "RADIAN"):
raise ValueError("Series unit should be one of ('second', 'hertz', 'meter', or 'radian'")
raise ValueError("Series unit should be one of " +
"('second', 'hertz', 'meter', or 'radian'")
self._unit = value.upper()

def extend(self, other_axis):
Copy link
Member

Choose a reason for hiding this comment

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

The expectation with a method named extend is in-place extension, as in lists. If possible, I'd prefer not to violate these expectations. Could this implementation simply be renamed to __add__?

Expand Down Expand Up @@ -1270,10 +1304,12 @@ def __getitem__(self, item):
nelements = (idx_end - idx_start) // step
if nelements < 0:
nelements = 0
return Series(idx_start * self.step + self.start, self.step * step, nelements, self.unit)
return Series(idx_start * self.step + self.start, self.step * step,
nelements, self.unit)
elif isinstance(item, int):
return self.get_element(item)
raise IndexError('Series can only be indexed with integers or slices without breaking the regular structure')
raise IndexError('Series can only be indexed with integers or slices ' +
'without breaking the regular structure')

def get_element(self, index):
"""
Expand All @@ -1291,5 +1327,6 @@ def get_element(self, index):
if index < 0:
index = self.size + index
if index >= self.size:
raise IndexError("index %i is out of range for get_series with size %i" % (index, self.size))
raise IndexError("index %i is out of range for get_series with size %i" %
(index, self.size))
return self.start + self.step * index