Skip to content

tst: explicitly set intent codes to allow proper loading #604

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 8 commits into from
Mar 9, 2018
Merged

tst: explicitly set intent codes to allow proper loading #604

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b560da6
fix: explicitly set intent codes to allow proper loading
mgxd Feb 23, 2018
8d5d70d
fix: do not allow saving of images without intent
mgxd Feb 23, 2018
fc619c0
fix: use unknown CIFTI type, raise ValueError if incorrect intent code
mgxd Feb 26, 2018
4ab086b
enh: move cifti intent check into to_file_map
mgxd Mar 4, 2018
0036e0c
enh: set default intent code when saving CIFTI images
mgxd Mar 6, 2018
e995d50
STY: remove unused import
mgxd Mar 6, 2018
5f6b64f
sty: utilize assert_equals
mgxd Mar 6, 2018
ba81e30
fix: use correct CIFTI intent names
mgxd Mar 8, 2018
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
3 changes: 3 additions & 0 deletions nibabel/cifti2/cifti2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1384,6 +1384,9 @@ def to_file_map(self, file_map=None):
header = self._nifti_header
extension = Cifti2Extension(content=self.header.to_xml())
header.extensions.append(extension)
# if intent code is not set, default to unknown CIFTI
if header.get_intent()[0] == 'none':
header.set_intent('NIFTI_INTENT_CONNECTIVITY_UNKNOWN')
data = reshape_dataobj(self.dataobj,
(1, 1, 1, 1) + self.dataobj.shape)
# If qform not set, reset pixdim values so Nifti2 does not complain
Expand Down
63 changes: 54 additions & 9 deletions nibabel/cifti2/tests/test_new_cifti2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@
"""
import numpy as np

import nibabel as nib
from nibabel import cifti2 as ci
from nibabel.tmpdirs import InTemporaryDirectory

from nose.tools import assert_true, assert_equal
from nose.tools import assert_true, assert_equal, assert_raises

affine = [[-1.5, 0, 0, 90],
[0, 1.5, 0, -85],
Expand Down Expand Up @@ -212,10 +213,15 @@ def test_dtseries():
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(13, 9)
img = ci.Cifti2Image(data, hdr)
print(img.nifti_header.get_intent())
Copy link
Member

Choose a reason for hiding this comment

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

Remove print.

img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_DENSE_SERIES')

with InTemporaryDirectory():
ci.save(img, 'test.dtseries.nii')
img2 = ci.load('test.dtseries.nii')
img2 = nib.load('test.dtseries.nii')
assert_true(img2.nifti_header.get_intent()[0]
== 'dense data series/fiber fans')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal(img2.nifti_header.get_intent()[0], 'dense data series/fiber fans')

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
check_series_map(img2.header.matrix.get_index_map(0))
check_geometry_map(img2.header.matrix.get_index_map(1))
Expand All @@ -231,10 +237,13 @@ def test_dscalar():
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(2, 9)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_DENSE_SCALARS')

with InTemporaryDirectory():
ci.save(img, 'test.dscalar.nii')
img2 = ci.load('test.dscalar.nii')
img2 = nib.load('test.dscalar.nii')
assert_true(img2.nifti_header.get_intent()[0] == 'dense scalar')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
check_scalar_map(img2.header.matrix.get_index_map(0))
check_geometry_map(img2.header.matrix.get_index_map(1))
Expand All @@ -250,10 +259,13 @@ def test_dlabel():
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(2, 9)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_DENSE_LABELS')

with InTemporaryDirectory():
ci.save(img, 'test.dlabel.nii')
img2 = ci.load('test.dlabel.nii')
img2 = nib.load('test.dlabel.nii')
assert_true(img2.nifti_header.get_intent()[0] == 'dense label')
assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
check_label_map(img2.header.matrix.get_index_map(0))
check_geometry_map(img2.header.matrix.get_index_map(1))
Expand All @@ -267,10 +279,13 @@ def test_dconn():
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(9, 9)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_DENSE')

with InTemporaryDirectory():
ci.save(img, 'test.dconn.nii')
img2 = ci.load('test.dconn.nii')
img2 = nib.load('test.dconn.nii')
assert_true(img2.nifti_header.get_intent()[0] == 'dense connectivity')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
assert_equal(img2.header.matrix.get_index_map(0),
img2.header.matrix.get_index_map(1))
Expand All @@ -287,10 +302,14 @@ def test_ptseries():
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(13, 3)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_PARCELLATED_SERIES')

with InTemporaryDirectory():
ci.save(img, 'test.ptseries.nii')
img2 = ci.load('test.ptseries.nii')
img2 = nib.load('test.ptseries.nii')
assert_true(img2.nifti_header.get_intent()[0]
== 'parcellated data series')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
check_series_map(img2.header.matrix.get_index_map(0))
check_parcel_map(img2.header.matrix.get_index_map(1))
Expand All @@ -306,10 +325,13 @@ def test_pscalar():
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(2, 3)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_PARCELLATED_SCALAR')

with InTemporaryDirectory():
ci.save(img, 'test.pscalar.nii')
img2 = ci.load('test.pscalar.nii')
img2 = nib.load('test.pscalar.nii')
assert_true(img2.nifti_header.get_intent()[0] == 'parcellated scalar')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
check_scalar_map(img2.header.matrix.get_index_map(0))
check_parcel_map(img2.header.matrix.get_index_map(1))
Expand All @@ -325,10 +347,14 @@ def test_pdconn():
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(2, 3)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_PARCELLATED_DENSE')

with InTemporaryDirectory():
ci.save(img, 'test.pdconn.nii')
img2 = ci.load('test.pdconn.nii')
assert_true(img2.nifti_header.get_intent()[0]
== 'parcellated dense connectivity')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
check_geometry_map(img2.header.matrix.get_index_map(0))
check_parcel_map(img2.header.matrix.get_index_map(1))
Expand All @@ -344,10 +370,14 @@ def test_dpconn():
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(2, 3)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_DENSE_PARCELLATED')

with InTemporaryDirectory():
ci.save(img, 'test.dpconn.nii')
img2 = ci.load('test.dpconn.nii')
assert_true(img2.nifti_header.get_intent()[0]
== 'dense parcellated connectivity')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
check_parcel_map(img2.header.matrix.get_index_map(0))
check_geometry_map(img2.header.matrix.get_index_map(1))
Expand All @@ -367,6 +397,9 @@ def test_plabel():
with InTemporaryDirectory():
ci.save(img, 'test.plabel.nii')
img2 = ci.load('test.plabel.nii')
Copy link
Member Author

Choose a reason for hiding this comment

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

is this a valid CIFTI type? I cannot find *plabel.nii in the specifications
https://www.nitrc.org/forum/attachment.php?attachid=341&group_id=454&forum_id=1955

Copy link
Member

Choose a reason for hiding this comment

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

there is a plabel: https://github.com/Washington-University/workbench/blob/master/src/Commands/CommandParser.cxx#L661

but indeed i don't see it in the spec.

Copy link
Member

Choose a reason for hiding this comment

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

also here: https://github.com/Washington-University/workbench/blob/6c7414d9ba379604c10673d6d68eeeb68f1332e9/src/Common/DataFileTypeEnum.cxx#L207

Copy link
Member Author

Choose a reason for hiding this comment

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

If we will raise an exception upon saving, we have some options

  • remove the plabel test
  • alter ci.save's signature to include a new parameter, force=False, that can be switched on to allow saving without intent codes

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

how about skipping the plabel test for now till we find the INTENT code. i would rather not have a parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

@effigies i had no luck finding it as well
https://github.com/Washington-University/workbench/blob/1b79e569e8b8277f4abf71fabd4afc2889849362/src/FilesBase/nifti2.h#L32-L49

Copy link

@coalsont coalsont Mar 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

plabel is not in the cifti-2 spec. We didn't see it being useful at the time, so it was not in the draft. We later found that the information needed to reorder parcellated files to group certain parcels together in matrix display could be stored as a parcels by labels file, and decided to use that extention, matching the previous pattern of extension naming.

Per the "unknown" special cifti intent (main cifti-2 document, top of page 13, middle of paragraph), it is in fact valid to make such a file, and its extension is technically open to be anything that ends in ".<something>.nii". Feel free to write a test that loads and/or saves a cifti mapping combination that is not in the spec, this is an expected use case.

assert_true(img.nifti_header.get_intent()[0]
== 'dense fiber/fan samples')
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be unknown?

Copy link
Member Author

Choose a reason for hiding this comment

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

I matched it with what's consistent in the intent codes

nibabel/nibabel/cifti2/parse_cifti2.py

Line 55 in e48b746

(3000, 'dense fiber/fan samples', (), 'NIFTI_INTENT_CONNECTIVITY_UNKNOWN'),

Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

Copy link

Choose a reason for hiding this comment

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

There is something incorrect here (maybe more than one thing). Dense fiber samples and dense fan samples are just dscalar files with a specific number and meaning of maps - they use the dscalar intent code and name. Their extensions are .dfibersamp.nii and .dfansamp.nii. The dense fans file type is another instance of this.

parcel by label files (which we use the extension .plabel.nii for) are not a "standard" mapping combination, so they should use intent 3000, and intent name "ConnUnknown".

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
check_label_map(img2.header.matrix.get_index_map(0))
check_parcel_map(img2.header.matrix.get_index_map(1))
Expand All @@ -380,10 +413,14 @@ def test_pconn():
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(3, 3)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_PARCELLATED')

with InTemporaryDirectory():
ci.save(img, 'test.pconn.nii')
img2 = ci.load('test.pconn.nii')
assert_true(img.nifti_header.get_intent()[0]
== 'parcellated connectivity')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
assert_equal(img2.header.matrix.get_index_map(0),
img2.header.matrix.get_index_map(1))
Expand All @@ -394,17 +431,21 @@ def test_pconn():
def test_pconnseries():
parcel_map = create_parcel_map((0, 1))
series_map = create_series_map((2, ))

Copy link
Member

Choose a reason for hiding this comment

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

I'd re-add this line to clean the diff.

matrix = ci.Cifti2Matrix()
matrix.append(parcel_map)
matrix.append(series_map)
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(3, 3, 13)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_PARCELLATED_'
'PARCELLATED_SERIES')

with InTemporaryDirectory():
ci.save(img, 'test.pconnseries.nii')
img2 = ci.load('test.pconnseries.nii')
assert_true(img.nifti_header.get_intent()[0]
== 'parcellated connectivity series')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
assert_equal(img2.header.matrix.get_index_map(0),
img2.header.matrix.get_index_map(1))
Expand All @@ -416,17 +457,21 @@ def test_pconnseries():
def test_pconnscalar():
parcel_map = create_parcel_map((0, 1))
scalar_map = create_scalar_map((2, ))

Copy link
Member

Choose a reason for hiding this comment

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

Re-add.

matrix = ci.Cifti2Matrix()
matrix.append(parcel_map)
matrix.append(scalar_map)
hdr = ci.Cifti2Header(matrix)
data = np.random.randn(3, 3, 13)
img = ci.Cifti2Image(data, hdr)
img.nifti_header.set_intent('NIFTI_INTENT_CONNECTIVITY_PARCELLATED_'
'PARCELLATED_SCALAR')

with InTemporaryDirectory():
ci.save(img, 'test.pconnscalar.nii')
img2 = ci.load('test.pconnscalar.nii')
assert_true(img.nifti_header.get_intent()[0]
== 'parcellated connectivity scalar')
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal

assert_true(isinstance(img2, ci.Cifti2Image))
assert_true((img2.get_data() == data).all())
assert_equal(img2.header.matrix.get_index_map(0),
img2.header.matrix.get_index_map(1))
Expand Down