Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.84%. Comparing base(917afab) to head(af7265c).
Report is 1255 commits behind head on master.

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

@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 originalinfer_intent kwarg intovalidate, which checks for the presence of the expected IndexMaps (and also the correct order) on top of setting the intent code.

I believe the failing tests are unrelated - this is ready for review.

Yup, openedpydicom/pydicom#1214 for those failures.

Overall, I'm not sure I like this UX. If I try to save ageneric_cifti.nii withoutvalidate=False, this is going to give me aKeyError.

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.nii.gz, which we've previously allowed, even though CIFTI strongly discourages it.

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]exceptKeyError:expected_intent="NIFTI_INTENT_CONNECTIVITY_UNKNOWN"expected_ext=".nii"ifvalidateandexpected_intent!=found_intent:warnsetintentifvalidateandnotfname.endswith(expected_ext):warn

This logic is simplified, since axis type tuples are not unique determinants. (NIFTI_INTENT_CONNECTIVITY_DENSE_SERIES applies todtseries.nii anddfan.nii,(CIFTI_INDEX_TYPE_SCALARS, CIFTI_INDEX_TYPE_BRAIN_MODELS) applies todscalar.nii anddfan.nii, anddfibersamp.nii anddfansamp.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. usingwarnings.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.