ENH: Add option to infer CIFTI-2 intent codes #932
Conversation
Codecov Report
@@ Coverage Diff @@
## master #932 +/- ##
==========================================
- Coverage 91.91% 91.84% -0.07%
==========================================
Files 99 99
Lines 12514 12536 +22
Branches 2204 2208 +4
==========================================
+ Hits 11502 11514 +12
- Misses 678 684 +6
- Partials 334 338 +4
Continue to review full report at Codecov.
|
mgxd
force-pushed
the
enh/cifti-save
branch
2 times, most recently
from
71f6526
to
b55a363
Compare
|
@effigies Totally forgot about this PR - I liked your suggestion in #932 (comment) so I went ahead and added that. Since that now includes IndexMap information, I reworked the original |
mgxd
force-pushed
the
enh/cifti-save
branch
3 times, most recently
from
a6c3049
to
021e5de
Compare
|
I believe the failing tests are unrelated - this is ready for review. |
|
Yup, opened pydicom/pydicom#1214 for those failures. |
|
Overall, I'm not sure I like this UX. If I try to save a My inclination is that we want warnings, not errors, by default. And inferring intent codes and validating axes feel like they shouldn't be controlled by the same parameter. @MichielCottaar @satra Would be interested in your opinions (or anyone else who uses CIFTI a lot) on how we can be helpful without being burdensome. I haven't checked this, but I suspect that the logic will fail if someone tries to save a compressed |
|
Thanks for looking into these intent codes and filename extensions. I would suggest to turn around the logic to use the map types as a ground truth of what the user intends rather than the file extension. At least for the current CIFTI file types, it is possible to determine what the intent code should be for any set of map types. I think such an inference could just added in without most users noticing (ideally it would not overwrite any intent code explicitly set by the user). I do like the idea of also checking that the filename extension matches what is being stored. However, I agree with effigies that it is probably better to keep this as a warning. |
|
I think using the axis types as a more authoritative indicator of intent than the extension makes a lot of sense. So something like: try:
expected_intent = CIFTI_CODES.niistring[axis_types]
expected_ext = CIFTI_CODES.extension[axis_types]
except KeyError:
expected_intent = "NIFTI_INTENT_CONNECTIVITY_UNKNOWN"
expected_ext = ".nii"
if validate and expected_intent != found_intent:
warn
set intent
if validate and not fname.endswith(expected_ext):
warnThis logic is simplified, since axis type tuples are not unique determinants. ( This would also treat With this mode, I think I'm okay with controlling both behaviors with a single parameter. |
|
@effigies - your plan looks reasonable. the one thing perhaps to think a bit more about is exception vs warning. warnings in python are mostly useless, i think, in dataflows and large scale processing, which is where such mismatches are likely to show up. the question is whether a reasonable assumption is being made by the tool when processing when ignoring a warning (since warnings are never trapped by downstream code only filtered out). |
|
That's a fair point. What would be a case in which you'd want to see an error? Trying to think through what the default behaviors should be. I think there should be something between "error" and "no helping", so if we do want to promote these checks to exceptions, then I'm back to wanting separate switches for inferring the intent code and performing validation. |
|
this is probably going to take this to a different PR/problem space, so first i will say that true, false, warn/except is a good starting way of handling this. so the simple use cases of matrix dimension flips and others should be choices for exception (e.g., timeseries by vertices vs vertices by timeseries). although i'm not sure as i write this if cifti actually enforces this or hcp-flavored cifti uses this as convention (i think the latter). more general approach, let validate take a callable, and one could build up an hcp-flavored validator (this can check on extension to internal mismatches, dimension mismatches, even have heuristics for certain types of information depending on intent codes). most algorithms assume that the file given to it is the correct file instead of checking to see if it is. for example, i load a dtseries and i assume that the dimensions are vertices/voxels by time. but it may be impossible for nibabel in general to know what they intent of the algorithm/tool designer was. so let the tool designer decide what is valid for their application and nibabel, if it can validate basic things like dimensions, availability of appropriate metadata (e.g., a dtseries should have a samplingstep inside the header that is reasonable). |
|
@MichielCottaar @satra thanks for the reviews 😄
I like this!
This was my first thought when creating this, but looking back, it may be too strict of an enforcement - I'm fine with reducing the severity of "incorrect" CIFTIs to a warning. I think warning handling is important, but that can (should?) be handled by the specific workflow (i.e. using |
- Enabled by default, validation will parse the output filename for a valid CIFTI2 extension. - If found, the intent code of the image will be set. Also, the CIFTI2Header will be check for compliant index maps for the intent code
|
@mgxd Are you shooting to get this in for the release? |
|
yes, I was hoping to - but this failing test reveals a problem with the approach of looking up based on map configuration.
when trying to validate the configuration for a |
|
We might need to modify recorders to return multiple options if there are multiple matches. |
|
I think we'll need to push this off to another release. I've held up 3.2.0 for too long and almost certainly won't have time to review more this week. |
|
This would be good to get in soon, if possible. It will need rebasing due to mass application of blue. |
|
@mgxd I'm going to aim for a new feature release next week. I don't know if you'd like to take a stab at bringing this one up to date? |
|
@effigies I'll try, but no promises |
|
No worries. |
Adds support for
FileBasedImagemethodsto_filenameandinstance_to_filenameto support keyword args.Adds keyword arg
validateto CIFTI'sto_filenameandinstance_to_filenamemethods.False(default), the intent code will not be set (except when the intent code is"none")True, the first suffix of the output filename will be compared to valid suffixes from within the CIFTI-2 specification. If there is a match, the corresponding intent code will be set, and each IndexMap within the header matrix is checked.