Skip to content

[FIX] #2435 Error creating gradients in DTIRecon - #2460

Merged
effigies merged 2 commits into
nipy:masterfrom
mmromero:patch-1
Feb 23, 2018
Merged

[FIX] #2435 Error creating gradients in DTIRecon#2460
effigies merged 2 commits into
nipy:masterfrom
mmromero:patch-1

Conversation

@mmromero

@mmromero mmromero commented Feb 23, 2018

Copy link
Copy Markdown
Contributor

The bvecs file was read assuming that directions followed YZX order, which created a gradients_matrix.txt with swapped directions. The new order is XYZ.

Closes #2435.

The bvecs file was read assuming that directions followed YZX order, which created a gradients_matrix.txt with swapped directions. The new order is XYZ.

@effigies effigies left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this. If it's not too much trouble, I have a suggestion for additional cleanup.

bvecs_z = [
val for val in re.split('\s+',
fbvecs.readline().strip())
]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's just go ahead and simplify this while we're here:

with open(bvecs_file) as fbvecs:
    bvecs_x = fbvecs.readline().split()
    bvecs_y = fbvecs.readline().split()
    bvecs_z = fbvecs.readline().split()
@effigies effigies added this to the 1.0.1 milestone Feb 23, 2018
@effigies

Copy link
Copy Markdown
Member

Great, thanks. Can you verify that this still produces the output you need?

@mmromero

Copy link
Copy Markdown
Contributor Author

I did verify it before committing.

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #2460 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2460   +/-   ##
=======================================
  Coverage   66.62%   66.62%           
=======================================
  Files         328      328           
  Lines       42508    42508           
  Branches     5280     5277    -3     
=======================================
  Hits        28321    28321           
  Misses      13505    13505           
  Partials      682      682
Flag Coverage Δ
#smoketests 50.73% <ø> (ø) ⬆️
#unittests 63.95% <0%> (ø) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/diffusion_toolkit/dti.py 63.88% <0%> (ø) ⬆️

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 cc750f4...0262588. Read the comment docs.

@effigies

Copy link
Copy Markdown
Member

I'll merge this in a batch with some others soon. Do you want to add yourself to the .zenodo.json? You'll be listed as a contributor in the Zenodo entry for the next release.

@effigies

Copy link
Copy Markdown
Member

Merging. Please update Zenodo in another PR, if you would like to be listed as a contributor.

@effigies
effigies merged commit ae10756 into nipy:master Feb 23, 2018
mmromero added a commit to mmromero/nipype that referenced this pull request Feb 24, 2018
I added myself as contributor after reporting issue nipy#2435 and PR nipy#2460
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants