FIX: Avoid diverting CIFTI dtype from original BOLD #532

mgxd · 2020-06-02T15:49:34Z

Since outputs would change, this PR targets 1.3.0

pull-assistant · 2020-06-02T15:49:36Z

Best reviewed: with all changes

Optimal code review plan (2 commits squashed)

FIX: Avoid diverting... ... ENH: apply changes f...

Squashed 2 commits:

FIX: Avoid diverting dtype from original

ENH: apply changes from @effigies review

Powered by Pull Assistant. Last update c5a257f ... 915cb01. Read the comment docs.

niworkflows/interfaces/cifti.py

codecov · 2020-06-02T16:39:26Z

Codecov Report

Merging #532 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
- Coverage   64.17%   64.16%   -0.02%     
==========================================
  Files          43       43              
  Lines        5290     5291       +1     
  Branches      770      770              
==========================================
  Hits         3395     3395              
- Misses       1750     1751       +1     
  Partials      145      145

Flag	Coverage Δ
#documentation	`33.84% <0.00%> (-0.01%)`	⬇️
#reportlettests	`100.00% <ø> (ø)`
#travis	`59.29% <0.00%> (-0.02%)`	⬇️
#unittests	`47.58% <0.00%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
niworkflows/interfaces/cifti.py	`53.84% <0.00%> (-0.35%)`	⬇️

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 a6a328f...c5a257f. Read the comment docs.

mgxd

a few comments to explain some changes

niworkflows/interfaces/cifti.py

effigies · 2020-06-02T17:09:28Z

niworkflows/interfaces/cifti.py

    label_img = _reorient_image(label_img, orientation="LAS")

-    bold_data = bold_img.get_fdata(dtype="float32")
+    bold_data = bold_img.get_fdata()


Is there a good reason to upcast to float64?

nope, can revert that

niworkflows/interfaces/cifti.py

effigies · 2020-06-02T17:45:39Z

niworkflows/interfaces/cifti.py

                medial = np.nonzero(annot.darrays[0].data)[0]
            # extract values across volumes
-            ts = np.array([tsarr.data[medial] for tsarr in surf.darrays])
+            ts = np.array([tsarr.data[medial] for tsarr in surf_ts.darrays])


Oh, here would be a good place to use agg_data().

Suggested change

ts = np.array([tsarr.data[medial] for tsarr in surf_ts.darrays])

ts = surf_ts.agg_data()[medial, :]

doesn't agg_data() return a tuple?

If the image has multiple types of data array, or you request multiple intents, then no.

If you want to be safe (e.g., if we start putting surfaces inside functional files...):

Suggested change

ts = np.array([tsarr.data[medial] for tsarr in surf_ts.darrays])

ts = surf_ts.agg_data("time series")[medial, :]

FWIW this case is explicitly covered in the docs: https://nipy.org/nibabel/reference/nibabel.gifti.html#nibabel.gifti.gifti.GiftiImage.agg_data

running fmriprep with this change leads to:

ts = surf_ts.agg_data("time series")[medial, :] TypeError: tuple indices must be integers or slices, not tuple

I think keeping it as it was is simplest ATM

Sure. That seems really weird, though. If you can pdb it at some point, I would be interested in the details.

Oh, if there are no darrays, you do get an empty tuple.

First release in the 1.3.x series. This release includes enhancements and bug-fixes towards the release of the first LTS version of fMRIPrep. PyBIDS has been revised to use more recent versions, a series of ANTs' interfaces have been deemed ready to upstream into Nipype, and several improvements regarding multi-echo EPI are included. With thanks to Basile Pinsard for contributions. * FIX: Patch ``ApplyTransforms`` spec to permit identity in a chain (#554) * FIX: Add dots to extensions in PyBIDS' config file (#548) * FIX: Segmentation plots aligned with cardinal axes (#544) * FIX: Skip T1w file existence check if ``anat_derivatives`` are provided (#545) * FIX: Avoid diverting CIFTI dtype from original BOLD (#532) * ENH: Add ``smooth`` input to ``RegridToZooms`` (#549) * ENH: Enable ``DerivativesDataSink`` to take multiple source files to derive entities (#547) * ENH: Allow ``bold_reference_wf`` to accept multiple EPIs/SBRefs (#408) * ENH: Numerical stability of EPI brain-masks using probabilistic prior (#485) * ENH: Add a pure-Python interface to resample to specific resolutions (#511) * MAINT: Finalize upstreaming of ANTs' interfaces to Nipype (#550) * MAINT: Update to Python +3.6 (#541)

1.3.0rc3 First release in the 1.3.x series. This release includes enhancements and bug-fixes towards the release of the first LTS version of fMRIPrep. PyBIDS has been revised to use more recent versions, a series of ANTs' interfaces have been deemed ready to upstream into Nipype, and several improvements regarding multi-echo EPI are included. With thanks to Basile Pinsard for contributions. * FIX: Patch ``ApplyTransforms`` spec to permit identity in a chain (nipreps#554) * FIX: Add dots to extensions in PyBIDS' config file (nipreps#548) * FIX: Segmentation plots aligned with cardinal axes (nipreps#544) * FIX: Skip T1w file existence check if ``anat_derivatives`` are provided (nipreps#545) * FIX: Avoid diverting CIFTI dtype from original BOLD (nipreps#532) * ENH: Add ``smooth`` input to ``RegridToZooms`` (nipreps#549) * ENH: Enable ``DerivativesDataSink`` to take multiple source files to derive entities (nipreps#547) * ENH: Allow ``bold_reference_wf`` to accept multiple EPIs/SBRefs (nipreps#408) * ENH: Numerical stability of EPI brain-masks using probabilistic prior (nipreps#485) * ENH: Add a pure-Python interface to resample to specific resolutions (nipreps#511) * MAINT: Finalize upstreaming of ANTs' interfaces to Nipype (nipreps#550) * MAINT: Update to Python +3.6 (nipreps#541)

1.3.0 First release in the 1.3.x series. This release includes enhancements and bug-fixes towards the release of the first LTS (*long-term support*) version of *fMRIPrep*. *PyBIDS* has been revised to use more recent versions, a series of ANTs' interfaces have been deemed ready to upstream into *Nipype*, and several improvements regarding multi-echo EPI are included. With thanks to Basile Pinsard for contributions. * FIX: Patch ``ApplyTransforms`` spec to permit identity in a chain (nipreps#554) * FIX: Add dots to extensions in PyBIDS' config file (nipreps#548) * FIX: Segmentation plots aligned with cardinal axes (nipreps#544) * FIX: Skip T1w file existence check if ``anat_derivatives`` are provided (nipreps#545) * FIX: Avoid diverting CIFTI dtype from original BOLD (nipreps#532) * ENH: Add ``smooth`` input to ``RegridToZooms`` (nipreps#549) * ENH: Enable ``DerivativesDataSink`` to take multiple source files to derive entities (nipreps#547) * ENH: Allow ``bold_reference_wf`` to accept multiple EPIs/SBRefs (nipreps#408) * ENH: Numerical stability of EPI brain-masks using probabilistic prior (nipreps#485) * ENH: Add a pure-Python interface to resample to specific resolutions (nipreps#511) * MAINT: Upstream all bug-fixes in the 1.2.9 release * MAINT: Finalize upstreaming of ANTs' interfaces to Nipype (nipreps#550) * MAINT: Update to Python +3.6 (nipreps#541)

FIX: Avoid diverting dtype from original

c5a257f

mgxd added the api change label Jun 2, 2020

mgxd mentioned this pull request Jun 2, 2020

BOLD dtypes are larger than necessary nipreps/fmriprep#2157

Closed

effigies reviewed Jun 2, 2020

View reviewed changes

niworkflows/interfaces/cifti.py Outdated Show resolved Hide resolved

mgxd commented Jun 2, 2020

View reviewed changes

niworkflows/interfaces/cifti.py Outdated Show resolved Hide resolved

niworkflows/interfaces/cifti.py Show resolved Hide resolved

niworkflows/interfaces/cifti.py Outdated Show resolved Hide resolved

mgxd marked this pull request as ready for review June 2, 2020 17:08

effigies reviewed Jun 2, 2020

View reviewed changes

niworkflows/interfaces/cifti.py Outdated Show resolved Hide resolved

ENH: apply changes from @effigies review

915cb01

effigies reviewed Jun 2, 2020

View reviewed changes

mgxd force-pushed the fix/cifti-save branch from a551f2d to 915cb01 Compare June 2, 2020 18:13

effigies approved these changes Jun 2, 2020

View reviewed changes

oesteban added this to the 1.3.0 milestone Jun 4, 2020

oesteban merged commit 42748dc into nipreps:master Jun 9, 2020

	ts = np.array([tsarr.data[medial] for tsarr in surf_ts.darrays])
	ts = surf_ts.agg_data()[medial, :]

	ts = np.array([tsarr.data[medial] for tsarr in surf_ts.darrays])
	ts = surf_ts.agg_data("time series")[medial, :]

FIX: Avoid diverting CIFTI dtype from original BOLD #532

FIX: Avoid diverting CIFTI dtype from original BOLD #532

Uh oh!

Conversation

mgxd commented Jun 2, 2020

Uh oh!

pull-assistant bot commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mgxd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pull-assistant bot commented Jun 2, 2020 •

edited

Loading

codecov bot commented Jun 2, 2020 •

edited

Loading