Skip to content

ENH: Update NIfTI header dimensions for CIFTI-2 compliance #929

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.

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

Closed
wants to merge 1 commit into from

Conversation

mgxd
Copy link
Member

@mgxd mgxd commented Jul 2, 2020

Updates the CiftiImage.update_headers() method to configure the NIfTI header's dim to CIFTI-2 standards brought up in nipreps/fmriprep#2205

@codecov
Copy link

codecov bot commented Jul 2, 2020

Codecov Report

Merging #929 into master will decrease coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #929      +/-   ##
==========================================
- Coverage   91.85%   91.71%   -0.14%     
==========================================
  Files          96       96              
  Lines       12347    12352       +5     
  Branches     2176     2176              
==========================================
- Hits        11341    11329      -12     
- Misses        675      686      +11     
- Partials      331      337       +6     
Impacted Files Coverage Δ
nibabel/cifti2/cifti2.py 96.36% <100.00%> (-0.31%) ⬇️
nibabel/environment.py 75.00% <0.00%> (-20.00%) ⬇️
nibabel/casting.py 85.28% <0.00%> (-0.87%) ⬇️
nibabel/nifti1.py 91.66% <0.00%> (-0.76%) ⬇️
nibabel/dft.py 80.36% <0.00%> (-0.62%) ⬇️
nibabel/volumeutils.py 84.13% <0.00%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a7a913...d5cdfa3. Read the comment docs.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Let's also explain what we're doing in the docstring. Quoting the bit of spec that requires this would probably make sense.

I would also consider this a bug-fix, not an enhancement.

@@ -1484,8 +1485,14 @@ def update_headers(self):
>>> img.update_headers()
>>> img.nifti_header.get_data_shape() == (2, 3, 4)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> img.nifti_header.get_data_shape() == (2, 3, 4)
>>> img.nifti_header.get_data_shape() == (1, 1, 1, 1, 2, 3, 4)

Copy link
Member Author

Choose a reason for hiding this comment

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

this breaks the current API, is that fine?

Copy link
Member

Choose a reason for hiding this comment

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

In what sense? Does it change img.shape? If so, we should divorce Cifti2Image.shape from Cifti2Image.nifti_header.get_data_shape().

Comment on lines 1491 to +1495
self._nifti_header.set_data_shape(self._dataobj.shape)
_dims = np.ones((8), dtype=int)
_dims[0] = 7 if len(self._dataobj.shape) == 3 else 6
_dims[5:8] = (self._dataobj.shape + (1,))[:3]
self._nifti_header['dim'] = _dims
Copy link
Member

Choose a reason for hiding this comment

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

Updating the input to set_data_shape() ought to do the trick.

Suggested change
self._nifti_header.set_data_shape(self._dataobj.shape)
_dims = np.ones((8), dtype=int)
_dims[0] = 7 if len(self._dataobj.shape) == 3 else 6
_dims[5:8] = (self._dataobj.shape + (1,))[:3]
self._nifti_header['dim'] = _dims
self._nifti_header.set_data_shape((1, 1, 1, 1) + self._dataobj.shape)

Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't equivalent though? dim[0] should be either 6 or 7

Copy link
Member

Choose a reason for hiding this comment

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

dim0 indicates the length of dim[1:8]. This wlll be determined by this tuple.

@effigies
Copy link
Member

effigies commented Jul 2, 2020

Closing in favor of #930, which targets maint/3.1.x.

@effigies effigies closed this Jul 2, 2020
@mgxd mgxd deleted the enh/cifti2-nifti-dims branch July 2, 2020 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants