tst: explicitly set intent codes to allow proper loading #604
Conversation
|
It might make sense also to add a warning (or exception) when saving |
| @@ -367,6 +384,7 @@ def test_plabel(): | |||
| with InTemporaryDirectory(): | |||
| ci.save(img, 'test.plabel.nii') | |||
| img2 = ci.load('test.plabel.nii') | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
how about skipping the plabel test for now till we find the INTENT code. i would rather not have a parameter.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
Codecov Report
@@ Coverage Diff @@
## master #604 +/- ##
=========================================
+ Coverage 94.48% 94.5% +0.01%
=========================================
Files 177 177
Lines 25078 25116 +38
Branches 2667 2668 +1
=========================================
+ Hits 23696 23735 +39
+ Misses 908 907 -1
Partials 474 474
Continue to review full report at Codecov.
|
|
Can we not add the plabel intent? The link @satra posted indicated it
exists as a code, though I didn't explore enough to identify the numerical
value.
On Feb 23, 2018 5:34 PM, "Mathias Goncalves" <[email protected]> wrote:
*@mgxd* commented on this pull request.
------------------------------
In nibabel/cifti2/tests/test_new_cifti2.py
<#604 (comment)>:
@@ -367,6 +384,7 @@ def test_plabel():
with InTemporaryDirectory():
ci.save(img, 'test.plabel.nii')
img2 = ci.load('test.plabel.nii')
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?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#604 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFF8qvKB3NKfACwHUno8KSLfbLu3ASjks5tXzzsgaJpZM4SRgba>
.
|
|
there doesn't seem to be a corresponding code for the plabel: https://github.com/Washington-University/workbench/blob/1b79e569e8b8277f4abf71fabd4afc2889849362/src/FilesBase/nifti2.h#L34 |
|
I guess I'd default to We could ping the HCP list about whether there should be an assigned intent code. |
nibabel/cifti2/cifti2.py
Outdated
| @@ -1442,4 +1442,6 @@ def save(img, filename): | |||
| filename : str | |||
| filename to which to save image | |||
| """ | |||
| if img.nifti_header.get_intent()[0] == 'none': | |||
| raise AttributeError("CIFTI image has an invalid intent code.") | |||
There was a problem hiding this comment.
Actually, I think I would rather set the intent by default to 'NIFTI_INTENT_CONNECTIVITY_UNKNOWN', and I think we should do this in Cifti2Image.to_file_map(), maybe after resetting pixdim.
if header.get_intent()[0] == 'none':
header.set_intent('NIFTI_INTENT_CONNECTIVITY_UNKNOWN')This feels a little cleaner to me, as the CIFTI equivalent of saving a NIFTI without setting an intent code, which we also allow.
|
@coalsont - what do you think? |
|
The intent code is completely automatic in our c++ cifti implementation. They are specified for specific XML setups, and the XML is known at time of file writing. So, we call a helper function to generate the correct intent code/string at header writing time: |
|
Additionally, it may be more robust to identify Cifti by the presence of a cifti extension, which is vital to the format, while we actually ignore the intent code on reading. Of course, when workbench actually doesn't know the file type in advance, we merely use the filename extension, which is even less robust. We added a warning for when a file is saved with an incorrect extension because of this: |
|
we should start creating a higher level API that addresses creating the different filetypes with appropriate intent. but at the lower level i would prefer raising an |
nibabel/cifti2/cifti2.py
Outdated
| @@ -1442,4 +1442,6 @@ def save(img, filename): | |||
| filename : str | |||
| filename to which to save image | |||
| """ | |||
| if img.nifti_header.get_intent()[0] == 'none': | |||
| raise ValueError("CIFTI image has an invalid intent code.") | |||
There was a problem hiding this comment.
Even if we're not going to auto-set the intent code, I think this check should be in Cifti2Image.to_file_map(). That way it's hit whether you use nb.cifti2.save, nb.save, or img.to_filename.
There was a problem hiding this comment.
Also, you should exercise this code in the test. Use assert_raises. Easy to stick immediately before a set_intent, like:
with assert_raises(ValueError):
img.to_filename(...)
img.nifti_header.set_intent(...)
img.to_filename(...)
| @@ -362,11 +379,13 @@ def test_plabel(): | |||
| matrix.append(parcel_map) | |||
| hdr = ci.Cifti2Header(matrix) | |||
| data = np.random.randn(2, 3) | |||
| img = ci.Cifti2Image(data, hdr) | |||
| img = ci.Cifti2Image(data, hdr | |||
|
So, just some perspective here: the cifti-2 specification intended to say that if you use the wrong intent code for the XML in the file, then the cifti file is malformed (though to us, it isn't particularly important), so even people using the low-level interface probably won't have a reason to specify an intent code manually. The cifti processing commands in workbench generally don't care about which of the standard cifti types you are working on (or even if it is a nonstandard type) - they have either an explicitly provided or a hardcoded implicit dimension to operate on, and if the mapping type for an important dimension doesn't match what they need to operate, they throw an error. In essence, operating on a low-level interface gives our processing commands more flexibility (spatial smoothing can be done on dscalar, dtseries, dconn, pdconn, and dpconn, using only one function signature taking a generic CiftiFile and a dimension specifier, rather than needing separate functions to take those files as different high-level types), and having them need to remember to set the intent code in their outputs would be a hassle and error-prone. Only our display logic needs to know how to specially handle specific file types (dscalar is presented as maps to cycle through, connectivity types load a row when clicked, etc). I don't know how well this maps onto how you plan for people to use your interface, but I don't see a compelling reason not to solve this format detail at the low level. |
|
@coalsont - thanks for chiming in. the only way our code currently recognizes a nifti-2 file as being a cifti file is based on the nifti intent code. thus if the intent is not set when someone saves a file created using the current low-level API, the nibabel load function returns a a higher level api will indeed try to do some operations agnostic of the intent type, but others will guide what sort of metadata as prescribed by the standard are compulsory vs not. |
|
@mgxd - the current error is happening in a place that automates testing certain file types. so one option is to set intent to unknown by default and creating a warning for the user when saving a file with type unknown instead of raising an exception. |
|
Thoughts on having the nifti-2 code look for a header extension with the cifti extension code? Without that extension, it simply can't be interpreted as cifti, and nothing else should use that extension code. If you aren't intending for the low-level code to ever be used by anything but the higher-level API (or people that specifically want to make malformed files), then I suppose it doesn't matter which of them figures out the correct intent code. |
|
The "NIFTI_INTENT_CONNECTIVITY_UNKNOWN" code is in fact explicitly valid and acceptable for cifti file types that aren't one of the standard mapping combinations - this is to allow flexibility beyond the "ordinary" types (want to make a series by series file? no problem!), without having to issue an update to the standard. Having it always trigger a warning may not be a good idea. |
|
Sounds good to me. With regard to checking XML headers to make sure they match the intent codes, I think that would also be reasonable at this level (though I can see an argument for a higher level interface doing that work, as well). I think that's beyond the scope of this PR, but if we do want to have that discussion it may be worth opening an issue. |
|
|
||
| with InTemporaryDirectory(): | ||
| ci.save(img, 'test.plabel.nii') | ||
| img2 = ci.load('test.plabel.nii') | ||
| assert_true(img.nifti_header.get_intent()[0] | ||
| == 'dense fiber/fan samples') |
There was a problem hiding this comment.
I matched it with what's consistent in the intent codes
nibabel/nibabel/cifti2/parse_cifti2.py
Line 55 in e48b746
There was a problem hiding this comment.
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".
| 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') |
There was a problem hiding this comment.
assert_equal(img2.nifti_header.get_intent()[0], 'dense data series/fiber fans')| @@ -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()) | |||
|
|
||
| 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') |
|
|
||
| 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') |
| img2 = ci.load('test.ptseries.nii') | ||
| img2 = nib.load('test.ptseries.nii') | ||
| assert_true(img2.nifti_header.get_intent()[0] | ||
| == 'parcellated data series') |
|
|
||
| with InTemporaryDirectory(): | ||
| ci.save(img, 'test.pconn.nii') | ||
| img2 = ci.load('test.pconn.nii') | ||
| assert_true(img.nifti_header.get_intent()[0] | ||
| == 'parcellated connectivity') |
| @@ -394,17 +431,21 @@ def test_pconn(): | |||
| def test_pconnseries(): | |||
| parcel_map = create_parcel_map((0, 1)) | |||
| series_map = create_series_map((2, )) | |||
|
|
|||
There was a problem hiding this comment.
I'd re-add this line to clean the diff.
|
|
||
| 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') |
|
|
||
| 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') |
| @@ -416,17 +457,21 @@ def test_pconnseries(): | |||
| def test_pconnscalar(): | |||
| parcel_map = create_parcel_map((0, 1)) | |||
| scalar_map = create_scalar_map((2, )) | |||
|
|
|||
|
I agree with @coalsont (#604 (comment)) that "dense fiber/fan samples" doesn't make much sense and isn't really justified by the links immediately above its entry in the recoder. nibabel/nibabel/cifti2/parse_cifti2.py Lines 51 to 76 in e48b746 These were added in 7551209, with no explanation for the names chosen. I would suggest we move to Is there any argument for keeping the current names? (I'm also open to this being a separate PR. We are moving beyond the scope of what @mgxd was here to do.) |
|
Thanks, that looks good to me. I'm comfortable with this PR as it is, but there is one thing I noticed while looking through the CIFTI sources: CIFTI-2 seems to very strongly tie intent codes and the intent name field. Neither NIFTI-1 nor NIFTI-2 constrain However, if we move the default setting to def update_headers(self):
"""...DOCSTRING..."""
header = self._nifti_header
header.set_data_shape(self._dataobj.shape)
# if intent code is not set, default to unknown CIFTI
if header.get_intent()[0] == 'none':
header.set_intent('NIFTI_INTENT_CONNECTIVITY_UNKNOWN')
header._structarr['intent_name'] = header.get_intent()[0]This could be a simple addition to @coalsont If you could comment on whether my read of @satra @matthew-brett @mgxd Does this seem reasonable to you? Or does this make more sense to push into the higher-level API, whenever that happens? Also, I recognize that Also, sorry if I keep moving the goal posts. Please say so if this is getting too quibbly. As I said, I think this looks good without further changes. |
|
@effigies - just to say - I'm happy if you're happy, feel free to merge when you think it's ready. |
|
Just a response on the intent code/name question: cifti does treat them as being linked. It seemed simple and useful to have intent_name be a human-readable form of intent_code, as our software leaves intent_name blank for volume files, because it doesn't know or care whether the volume is T1w, diffusion, or what atlas the warpfield is intended to align to (which are some examples of suggested intent_name usage from nifti-1.h). So, modifying Nifti2Header to do this cifti-specific intent code/name stuff is probably not the right place to do it - the obvious way to me is for the code that figures out the correct intent_code for the cifti file to also return the linked intent_name. This is squarely in the CiftiFile section of our c++ implementation (which naturally has access to the CiftiXML object, which is where we put the ugly code that checks for the combinations), and the NiftiHeader just accepts whatever we tell it to put in there: https://github.com/Washington-University/CiftiLib/blob/master/src/CiftiFile.cxx#L575-L576 However, I should explain that in our code, the CiftiXML object is not merely responsible for the XML parsing, but also as the friendly interface to the mapping information and dimensions, and as the main object for initializing a new CiftiFile for writing. |
related to #603
without explicitly setting the intent code in the header, CIFTI files opened through
nibabel.loadbecomeNifti2Images. Essentially this adds to the "documentation" of creating a CIFTI from scratch.Perhaps this can automated somehow within
ci.save()in the future