Skip to content
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

BF: Ensure BOLD and label orientations are equal #477

Merged
merged 5 commits into from Mar 17, 2020

Conversation

@mgxd
Copy link
Contributor

mgxd commented Mar 16, 2020

No description provided.

@pull-assistant

This comment has been minimized.

Copy link

pull-assistant bot commented Mar 16, 2020

Score: 0.79

Best reviewed: commit by commit


Optimal code review plan (1 warning)

ENH: Reorient volumetric data to LAS

...iworkflows/interfaces/cifti.py 48% changes removed in doc+sty: suggestions...

     sty: placate py35

     tst: add doctest

     doc+sty: suggestions from review

     ENH: return image if desired orientation already set

Powered by Pull Assistant. Last update 399cc41 ... e1a8a12. Read the comment docs.

@mgxd mgxd changed the title ENH: Reorient volumetric data to LAS BF: Ensure BOLD and label orientations are equal Mar 16, 2020
@mgxd

This comment has been minimized.

Copy link
Contributor Author

mgxd commented Mar 16, 2020

We were assuming the BOLD image and label image had a shared orientation

ijk = np.nonzero(label_data == label)
if ijk[0].size == 0: # skip label if nothing matches
continue
ts = (bold_data[ijk] if ts is None
else np.concatenate((ts, bold_data[ijk])))
vox += [[ijk[0][ix], ijk[1][ix], ijk[2][ix]]
for ix, row in enumerate(ts)]

However, this wasn't always the case - currently, the BOLD image is in RAS while the label image is in LAS. This adds a check to ensure the orientations match up, and if they don't, convert to LAS (HCP Pipelines default, although https://www.nitrc.org/forum/attachment.php?attachid=342&group_id=454&forum_id=1955 recommends RAS space).

And we can see the difference
before PR
post PR

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #477 into maint/1.1.x will increase coverage by 0.06%.
The diff coverage is 91.66%.

Impacted file tree graph

@@               Coverage Diff               @@
##           maint/1.1.x     #477      +/-   ##
===============================================
+ Coverage        62.86%   62.93%   +0.06%     
===============================================
  Files               41       41              
  Lines             4993     5005      +12     
  Branches           724      727       +3     
===============================================
+ Hits              3139     3150      +11     
- Misses            1703     1704       +1     
  Partials           151      151              
Flag Coverage Δ
#documentation 33.98% <8.33%> (-0.07%) ⬇️
#reportlettests 100.00% <ø> (ø)
#travis 57.78% <91.66%> (+0.08%) ⬆️
#unittests 46.03% <91.66%> (+0.10%) ⬆️
Impacted Files Coverage Δ
niworkflows/interfaces/cifti.py 54.54% <91.66%> (+3.13%) ⬆️

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 bfffaf8...e1a8a12. Read the comment docs.

@mgxd mgxd requested a review from oesteban Mar 17, 2020
niworkflows/interfaces/cifti.py Outdated Show resolved Hide resolved
niworkflows/interfaces/cifti.py Outdated Show resolved Hide resolved
niworkflows/interfaces/cifti.py Outdated Show resolved Hide resolved
niworkflows/interfaces/cifti.py Outdated Show resolved Hide resolved
@mgxd

This comment has been minimized.

Copy link
Contributor Author

mgxd commented Mar 17, 2020

any lingering concerns?

I'd like to get this out as a 1.1.11 despite it changing the outputs, since it's more of a bug fix imo (cc @effigies)

@mgxd mgxd changed the base branch from master to maint/1.1.x Mar 17, 2020
@effigies

This comment has been minimized.

Copy link
Collaborator

effigies commented Mar 17, 2020

Seems reasonable. In the long-term a fully-featured reorientation interface would be good to publish in nipype, but no need to go that far in this context.

@mgxd mgxd merged commit 3f1ae40 into nipreps:maint/1.1.x Mar 17, 2020
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/circleci: get_data Your tests passed on CircleCI!
Details
ci/circleci: test_package Your tests passed on CircleCI!
Details
ci/circleci: test_pytest Your tests passed on CircleCI!
Details
codecov/patch 91.66% of diff hit (target 62.86%)
Details
codecov/project 62.93% (+0.06%) compared to bfffaf8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mgxd mgxd deleted the mgxd:fix/cifti-orientation branch Mar 17, 2020
mgxd added a commit to poldracklab/fmriprep that referenced this pull request Mar 18, 2020
* CLI changes from #2028 were ported to `parser.py`

20.0.4 (March 17, 2020)
=======================
A bug-fix release improving documentation for filtering BIDS files and standardizing CIFTI volume orientation.

With thanks to Ursula Tooley for the contribution.

  * DOC: FAQ section for BIDS filter (#2028)
  * FIX: Ensure BOLD and label orientations are equal (`nipreps/niworkflows#477`_).

.. _`nipreps/niworkflows#477`: nipreps/niworkflows#477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.