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):
 warn

This logic is simplified, since axis type tuples are not unique determinants. (NIFTI_INTENT_CONNECTIVITY_DENSE_SERIES applies to dtseries.nii and dfan.nii, (CIFTI_INDEX_TYPE_SCALARS, CIFTI_INDEX_TYPE_BRAIN_MODELS) applies to dscalar.nii and dfan.nii, and dfibersamp.nii and dfansamp.nii share both intents and axes.)

This would also treat .nii.gz the same as a mismatched .<subext>.nii, and warn, but not fail, which feels a bit kinder to users.

With this mode, I think I'm okay with controlling both behaviors with a single parameter. validate seems fine to me...

@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 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

I like this!

warnings in python are mostly useless, i think, in dataflows and large scale processing, which is where such mismatches are likely to show up

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 warnings.simplefilter to treat them as errors).

@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.

(CIFTI_INDEX_TYPE_SCALARS, CIFTI_INDEX_TYPE_BRAIN_MODELS) applies to dscalar.nii and dfan.nii

when trying to validate the configuration for a *dscalar.nii file, the information for *dfan.nii is fetched. How can we avoid these collisions?

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.