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

ENH: Ensure subcortical CIFTI is in LAS orientation #484

Merged
merged 1 commit into from Mar 20, 2020

Conversation

mgxd
Copy link
Contributor

@mgxd mgxd commented Mar 19, 2020

Go one step beyond #477 and ensures volumetric output in LAS orientation.

@pull-assistant
Copy link

pull-assistant bot commented Mar 19, 2020

Score: 1.00

Best reviewed: commit by commit


Optimal code review plan

     ENH: Ensure subcortical CIFTI is in LAS orientation

Powered by Pull Assistant. Last update 097ad57 ... 097ad57. Read the comment docs.

@mgxd mgxd added this to the 1.2.0 milestone Mar 19, 2020
@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #484 into master will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
+ Coverage   62.86%   62.90%   +0.03%     
==========================================
  Files          41       41              
  Lines        5006     5003       -3     
  Branches      730      726       -4     
==========================================
  Hits         3147     3147              
+ Misses       1708     1705       -3     
  Partials      151      151              
Flag Coverage Δ
#documentation 33.99% <0.00%> (-0.01%) ⬇️
#masks ?
#reportlettests 100.00% <ø> (ø)
#travis 57.75% <0.00%> (-0.02%) ⬇️
#unittests 45.99% <0.00%> (-0.01%) ⬇️
Impacted Files Coverage Δ
niworkflows/interfaces/cifti.py 54.19% <0.00%> (-0.36%) ⬇️
niworkflows/interfaces/images.py 55.82% <0.00%> (+0.14%) ⬆️
niworkflows/viz/utils.py 80.06% <0.00%> (+0.24%) ⬆️
niworkflows/interfaces/mni.py 42.52% <0.00%> (+0.24%) ⬆️
niworkflows/utils/bids.py 77.17% <0.00%> (+0.82%) ⬆️

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 0277b6e...097ad57. Read the comment docs.

bold_img = _reorient_image(bold_img, target_img=label_img)
# ensure images match HCP orientation (LAS)
bold_img = _reorient_image(bold_img, orientation='LAS')
label_img = _reorient_image(label_img, orientation='LAS')
Copy link
Member

@effigies effigies Mar 19, 2020

Choose a reason for hiding this comment

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

Is this going to be the case for all label files? Should we have a table mapping space to orientation?

Copy link
Contributor Author

@mgxd mgxd Mar 20, 2020

Choose a reason for hiding this comment

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

yes, since the final output should always be in LAS we want to ensure both BOLD / labels are as well prior to setting the IJK voxels

Copy link
Member

@effigies effigies Mar 20, 2020

Choose a reason for hiding this comment

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

I guess my reading of Tim's response was that LAS was an accident of the orientation of a specific template. Are we guaranteed that all templates HCP ever uses will be in LAS?

Copy link
Contributor Author

@mgxd mgxd Mar 20, 2020

Choose a reason for hiding this comment

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

no, we aren't - but that seems excessive for us to worry about. I'd rather mirror the outputs generated from HCP Pipelines, which happen to include the volume in LAS orientation

@effigies
Copy link
Member

effigies commented Mar 20, 2020

Ok. We can make this more complicated when they do. Merge when you like.

@oesteban oesteban merged commit bbd65e1 into nipreps:master Mar 20, 2020
7 of 8 checks passed
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.

None yet

3 participants