[ENH] Reorient bvecs from affs #49

dPys · 2020-01-13T21:35:13Z

function + interface + 1st docstring test for reorienting vectors from/to rasb .tsv given a list of affine transforms.

…ine list

…ine transform list

pull-assistant · 2020-01-13T21:35:17Z

Best reviewed: commit by commit

Optimal code review plan (1 warning)

[ENH] Add API to vectors for reorienting bvecs from rasb .tsv and affi...

dmriprep/utils/vectors.py 67% changes removed in [ENH] Add API to vec...

[ENH] Add API to vectors for reorienting bvecs from rasb .tsv and affi...

Update dmriprep/interfaces/vectors.py

Powered by Pull Assistant. Last update 5f51f9d ... df7d72f. Read the comment docs.

pep8speaks · 2020-01-13T21:35:20Z

Hello @dPys, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 dmriprep.

Comment last updated at 2020-01-15 16:31:35 UTC

…ine transform list [FIX] Correct PEP8 violations

arokem

I believe this is what is causing Travis to be unhappy: https://travis-ci.org/nipreps/dmriprep/jobs/636585523#L563

dmriprep/interfaces/vectors.py

dPys · 2020-01-15T07:20:46Z

I believe this is what is causing Travis to be unhappy: https://travis-ci.org/nipreps/dmriprep/jobs/636585523#L563

Thanks for the review on this @arokem . Will try to incorporate your changes tomorrow and add some other updates to this proposed interface that I've made since. -D

Co-Authored-By: Ariel Rokem <arokem@gmail.com>

codecov · 2020-01-15T16:32:08Z

Codecov Report

Merging #49 into master will decrease coverage by 0.33%.
The diff coverage is 46.15%.

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
- Coverage   54.59%   54.26%   -0.34%     
==========================================
  Files          11       11              
  Lines         773      798      +25     
  Branches      116      117       +1     
==========================================
+ Hits          422      433      +11     
- Misses        350      364      +14     
  Partials        1        1

Impacted Files	Coverage Δ
dmriprep/utils/vectors.py	`95.23% <12.5%> (-4.77%)`	⬇️
dmriprep/interfaces/vectors.py	`88.52% <61.11%> (-11.48%)`	⬇️

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 5282950...df7d72f. Read the comment docs.

arokem · 2020-01-15T18:06:11Z

dmriprep/interfaces/vectors.py

+    >>> res = reor_vecs.run()
+    >>> out_rasb = res.outputs.out_rasb
+    >>> out_rasb_mat = np.loadtxt(out_rasb, skiprows=1)
+    >>> npt.assert_equal(oldrasb_mat, out_rasb_mat)


We need to import npt before reaching this line (see: https://travis-ci.org/nipreps/dmriprep/jobs/637522010#L577)

On further thought, maybe replace this with something like:

>>> oldrasb_mat == out_rasb_mat True

oesteban

I would keep squeezing the data structure of Vectors

oesteban · 2020-01-17T18:01:35Z

dmriprep/utils/vectors.py

@@ -375,3 +375,50 @@ def bvecs2ras(affine, bvecs, norm=True, bvec_norm_epsilon=0.2):
        rotated_bvecs[~b0s] /= norms_bvecs[~b0s, np.newaxis]
        rotated_bvecs[b0s] = np.zeros(3)
    return rotated_bvecs
+
+
+def reorient_vecs_from_ras_b(rasb_file, affines, b0_threshold=B0_THRESHOLD):


Can we make it a method of the vectors object (and therefore, it will only take the affines parameter)? Also drop the b0_threshold argument, since that should set only once in the vectors class.

oesteban · 2020-01-17T18:01:45Z

dmriprep/utils/vectors.py

+    might cause systematic bias in rotationally invariant measures, such as FA
+    and MD, and also cause characteristic biases in tractography, unless the
+    gradient directions are appropriately reoriented to compensate for this
+    effect [Leemans2009]_.


Undefined reference

Pruning out since I think it's extraneous here.

oesteban · 2020-01-17T18:04:14Z

dmriprep/interfaces/vectors.py

+        cwd = Path(runtime.cwd).absolute()
+        reor_rasb_file = fname_presuffix(
+            self.inputs.in_rasb, use_ext=False, suffix='_reoriented.tsv',
+            newpath=str(cwd))
+        np.savetxt(str(reor_rasb_file), reor_table,
+                   delimiter='\t', header='\t'.join('RASB'),
+                   fmt=['%.8f'] * 3 + ['%g'])
+
+        self._results['out_rasb'] = reor_rasb_file


I would use a Vector instance here and just call the reorient member (if transferred there, see other comment).

dPys added 6 commits January 13, 2020 12:43

[ENH] Add API to vectors for reorienting bvecs from rasb .tsv and aff…

5f51f9d

…ine list

[ENH] Add API to vectors for reorienting bvecs from rasb .tsv and aff…

e4e8f82

…ine list

[ENH] Add API to vectors for reorienting bvecs from rasb .tsv and aff…

b4f9976

…ine list

[ENH] Add API to vectors for reorienting bvecs from rasb .tsv and aff…

153fa1b

…ine list

[ENH] Add API to vectors for reorienting bvecs from rasb .tsv and aff…

fdab07a

…ine list

[ENH] Add API to vectors for reorienting bvecs from rasb .tsv and aff…

b150745

…ine transform list

dPys force-pushed the reorient_bvecs_from_affs branch from ecfd113 to 513764d Compare January 13, 2020 21:46

[ENH] Add API to vectors for reorienting bvecs from rasb .tsv and aff…

03829b3

…ine transform list [FIX] Correct PEP8 violations

dPys force-pushed the reorient_bvecs_from_affs branch from 513764d to 03829b3 Compare January 13, 2020 21:49

arokem reviewed Jan 15, 2020

View reviewed changes

dmriprep/interfaces/vectors.py Outdated Show resolved Hide resolved

dmriprep/interfaces/vectors.py Outdated Show resolved Hide resolved

dPys changed the title ~~Reorient bvecs from affs~~ [ENH] Reorient bvecs from affs Jan 15, 2020

dPys and others added 2 commits January 15, 2020 08:31

Update dmriprep/interfaces/vectors.py

11134a0

Co-Authored-By: Ariel Rokem <arokem@gmail.com>

Update dmriprep/interfaces/vectors.py

df7d72f

Co-Authored-By: Ariel Rokem <arokem@gmail.com>

arokem reviewed Jan 15, 2020

View reviewed changes

oesteban mentioned this pull request Jan 17, 2020

[WIP] Eddy Motion Correction (EMC) #62

Closed

oesteban reviewed Jan 17, 2020

View reviewed changes

dPys closed this Feb 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Reorient bvecs from affs #49

[ENH] Reorient bvecs from affs #49

dPys commented Jan 13, 2020

pull-assistant bot commented Jan 13, 2020 •

edited

Loading

pep8speaks commented Jan 13, 2020 •

edited

Loading

arokem left a comment

dPys commented Jan 15, 2020

codecov bot commented Jan 15, 2020

arokem Jan 15, 2020

arokem Jan 15, 2020

oesteban left a comment

oesteban Jan 17, 2020

dPys Jan 20, 2020

oesteban Jan 17, 2020

dPys Jan 20, 2020

oesteban Jan 17, 2020

[ENH] Reorient bvecs from affs #49

[ENH] Reorient bvecs from affs #49

Conversation

dPys commented Jan 13, 2020

pull-assistant bot commented Jan 13, 2020 • edited Loading

pep8speaks commented Jan 13, 2020 • edited Loading

Comment last updated at 2020-01-15 16:31:35 UTC

arokem left a comment

Choose a reason for hiding this comment

dPys commented Jan 15, 2020

codecov bot commented Jan 15, 2020

Codecov Report

arokem Jan 15, 2020

Choose a reason for hiding this comment

arokem Jan 15, 2020

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

oesteban Jan 17, 2020

Choose a reason for hiding this comment

dPys Jan 20, 2020

Choose a reason for hiding this comment

oesteban Jan 17, 2020

Choose a reason for hiding this comment

dPys Jan 20, 2020

Choose a reason for hiding this comment

oesteban Jan 17, 2020

Choose a reason for hiding this comment

pull-assistant bot commented Jan 13, 2020 •

edited

Loading

pep8speaks commented Jan 13, 2020 •

edited

Loading