This is WIP fixing #2606.

Changes proposed in this PR:

• import the module geomstats to perform computations on manifolds, like the space of frame displacements in 3D which is the Special Euclidean group in 3D or SE(3)
• modify FramewiseDisplacement to compute the displacement of a frame as a geodesic distance on the space of frames SE(3).

TODO:

• investigate what is the current parameterization of the frames, i.e. how is the translation+rotation represented in the vectors of mpars @oesteban ?
• adapt unit tests + run make check-before-commit

@ninamiolane - this would be a nice fix. it's a little closer to the implementation in artifactdetect i think.

https://github.com/nipy/nipype/blob/master/nipype/algorithms/rapidart.py#L103

perhaps to keep compatibility with the original FramewiseDisplacement, we could create a new class here or add an option to the existing class. also since this would be a dependency on geomstats we would make that optional and imported inside the class rather than making it a nipype dependency.

i will have to play with geomstats but it looks like a really nice package.

Thanks for the PR @ninamiolane!

+1 for making this optional and keeping the default behavior (there is too much software depending on FD calculated in a particular way).

Codecov Report

Merging #2607 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2607   +/-   ##
=======================================
  Coverage   66.91%   66.91%           
=======================================
  Files         340      340           
  Lines       43391    43391           
  Branches     5382     5382           
=======================================
  Hits        29037    29037           
  Misses      13647    13647           
  Partials      707      707

Flag	Coverage Δ
#smoketests	50.5% <0%> (ø)	:arrow_up:
#unittests	64.05% <0%> (ø)	:arrow_up:

---

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 c9a896c...4177f4e. Read the comment docs.

Hi @ninamiolane, do you want to try to get this in for the next release (1.1.1)? I suspect we could get this done in a couple days.

Please keep me in the loop, I'll be more than happy to help fix the tests or whatever you may need from me :)

Yes, that'd be great, thanks! Let me have another look tomorrow and I'll ping you if needed.

@oesteban @effigies could maybe one of you tell me which parameterization is used for the frames, i.e. for each line of mpars here: https://github.com/nipy/nipype/blob/master/nipype/algorithms/confounds.py#L310

Each line of mpars is 6D: 3 parameters for the translation and 3 parameters for the rotation. Could you tell me which parameter corresponds to what, i.e. which are for the translations, which are for the rotations and which parameterization is used for the rotations: axis-angle, euler angles (which euler angles?)..? I need to convert the mpars rotation parameterization to the axis-angle parameterization.

Thank you very much in advance.

These have been normalized to SPM format:

https://github.com/nipy/nipype/blob/4b2a8213a1ec19959410f46ecd5fb77368d9cdda/nipype/utils/misc.py#L227-L238

I believe they're in Euler angles, but perhaps @satra knows for certain here...

Hi @ninamiolane. Just a heads up that we'll try to make the next release on August 28, so it'd be great to try to get this in by August 20. Please let us know if you have any questions.

SPM seems to be using Euler angles to representation rotations, however it is not clear to me which Euler angle convention is used. (There are -at least?- 12 Euler angles conventions.)

@satra Would you know if the Euler angles used by SPM:

• are in extrinsic (fixed reference frame) or intrinsic coordinates (reference frame is moving with each successive elementary rotation along x, y, and z),
• are given in the order xyz or zyx (i.e. the order in which to perform the elementary rotations along x, y and z).

Thank you!

https://github.com/nipy/nipype/blob/4b2a8213a1ec19959410f46ecd5fb77368d9cdda/nipype/utils/misc.py#L227-L238

Hi Nina, I merged master, which sometimes helps clean up the diff, so that we could look at only the changes you've made. It worked this time, fortunately. To sync your branch, just use git pull before making any further changes.

The main remaining issue is establishing a ground truth for geodesic framewise displacement. The simplest approach would be simply to save the current output, so we'd get a regression test if something changes. Have you run this on some real data to make sure that the results you're getting are sensible?

Just a cursory glance at the error:

E       AssertionError: assert False
E        +  where False = <function allclose at 0x7fc211eca7b8>(array([0.0922165 , 0.040464  , 0.111654  , 0.274237  , 0.0615315 ,\n       0.037751  , 0.069177  , 0.0394165 , 0.066542...    0.034866  , 0.0435379 , 0.0189935 , 0.0270825 , 0.0893485 ,\n       0.0466895 , 0.048314  , 0.0224355 , 0.10331   ]), array([1.91347209e-02, 2.50928469e-02, 3.93226252e-02, 9.12766839e-02,\n       3.04741265e-02, 1.80387187e-02, 2.169073...1.08159609e-02, 3.14788339e-03, 1.62701165e-02,\n       1.59538550e-02, 1.71339482e-02, 1.77695202e-02, 4.47950018e-02]), atol=0.16)
E        +    where <function allclose at 0x7fc211eca7b8> = np.allclose
E        +    and   array([1.91347209e-02, 2.50928469e-02, 3.93226252e-02, 9.12766839e-02,\n       3.04741265e-02, 1.80387187e-02, 2.169073...1.08159609e-02, 3.14788339e-03, 1.62701165e-02,\n       1.59538550e-02, 1.71339482e-02, 1.77695202e-02, 4.47950018e-02]) = <function loadtxt at 0x7fc20f584c80>('/tmp/pytest-of-travis/pytest-0/popen-gw0/test_fd_riemannian0/fd.txt', skiprows=1)
E        +      where <function loadtxt at 0x7fc20f584c80> = np.loadtxt
E        +      and   '/tmp/pytest-of-travis/pytest-0/popen-gw0/test_fd_riemannian0/fd.txt' = \nfd_average = 0.027046558324051496\nout_figure = <undefined>\nout_file = /tmp/pytest-of-travis/pytest-0/popen-gw0/test_fd_riemannian0/fd.txt\n.out_file
E        +        where \nfd_average = 0.027046558324051496\nout_figure = <undefined>\nout_file = /tmp/pytest-of-travis/pytest-0/popen-gw0/test_fd_riemannian0/fd.txt\n = <nipype.interfaces.base.support.InterfaceResult object at 0x7fc1ec848358>.outputs

We're going from 0.092 to 0.019 (first entry) and 0.103 to 0.045 (final entry). It definitely seems plausible to see a 2-6x compression when going from an L1 in 6 dimensions to a more geometrically meaningful norm, but you have more experience here.

There's a secondary issue on Travis, which is that we cannot install geomstats on Python 3.7, as it depends on tensorflow, which currently does not have a binary distribution for 3.7. It appears this was resolved with tensorflow/tensorflow#21202, so we can expect this to fix itself with the next release. Fortunately, their release cycle looks like it's faster than monthly, so if we just keep an eye on that, we should be good to merge before 1.1.3.

So we're looking to release next Monday, and tensorflow still hasn't had a full release support Python 3.7. Additionally, it's probably not great for nipype to start semi-requiring tensorflow or pytorch for such a small functionality increment. I've submitted an issue to geomstats/geomstats#136, and would be happy to submit a patch if you'd like.

Any thoughts on the ground-truth file?

Hi @ninamiolane, @oesteban. Any chance of finishing this up this week?

@oesteban would you like to meet this week, for example thursday afternoon or friday afternoon? if not, next week?

Just a heads up: We're looking to make the next release on Nov 26, if you want to try to get this in this month.

Hi, just a reminder that the 1.1.8 release is targeted for January 28. Please let us know if you'd like to try to finish this up for that release.