ENH WIP: Use geodesic distance in FramewiseDisplacement #2607

ninamiolane · 2018-06-04T19:16:54Z

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

satra · 2018-06-04T19:27:01Z

@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.

nipype/algorithms/confounds.py

chrisgorgo · 2018-06-04T19:32:00Z

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-io · 2018-06-06T23:16:24Z

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 64.05%. Comparing base (9049156) to head (4177f4e).
Report is 1163 commits behind head on master.

Files with missing lines	Patch %	Lines
nipype/algorithms/confounds.py	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2607      +/-   ##
==========================================
- Coverage   64.85%   64.05%   -0.80%     
==========================================
  Files         299      297       -2     
  Lines       39510    39446      -64     
  Branches     5220     5212       -8     
==========================================
- Hits        25623    25267     -356     
- Misses      12832    13138     +306     
+ Partials     1055     1041      -14

Flag	Coverage Δ
unittests	`64.03% <95.23%> (-0.75%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

effigies · 2018-07-23T21:19:07Z

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.

oesteban · 2018-07-23T23:54:33Z

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

ninamiolane · 2018-07-24T17:29:43Z

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

ninamiolane · 2018-07-25T21:34:11Z

@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.

effigies · 2018-07-25T21:49:59Z

These have been normalized to SPM format:

nipype/nipype/utils/misc.py

Lines 227 to 238 in 4b2a821

    
           def normalize_mc_params(params, source): 
        
               """ 
        
               Normalize a single row of motion parameters to the SPM format. 
        
               SPM saves motion parameters as: 
        
                   x   Right-Left          (mm) 
        
                   y   Anterior-Posterior  (mm) 
        
                   z   Superior-Inferior   (mm) 
        
                   rx  Pitch               (rad) 
        
                   ry  Yaw                 (rad) 
        
                   rz  Roll                (rad) 
        
               """

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

effigies

Hi @ninamiolane. I took the liberty of reviewing this before you said it was ready. There are a few things that can be addressed before the final form of the geodesic metric is decided.

Also, could you merge the current master?

Please feel free to ask questions.

effigies · 2018-07-26T20:18:11Z

nipype/algorithms/confounds.py

@@ -300,16 +307,37 @@ class FramewiseDisplacement(BaseInterface):
        'tags': ['method']
    }]

-    def _run_interface(self, runtime):
+    def _run_interface(self, runtime, metric='L1'):


This metric parameter should be in FramewiseDisplacementInputSpec:

nipype/nipype/algorithms/confounds.py

Lines 229 to 258 in 1ffe23d

class FramewiseDisplacementInputSpec(BaseInterfaceInputSpec):

in_file = File(exists=True, mandatory=True, desc='motion parameters')

parameter_source = traits.Enum(

"FSL",

"AFNI",

"SPM",

"FSFAST",

"NIPY",

desc="Source of movement parameters",

mandatory=True)

radius = traits.Float(

50,

usedefault=True,

desc='radius in mm to calculate angular FDs, 50mm is the '

'default since it is used in Power et al. 2012')

out_file = File(

'fd_power_2012.txt', usedefault=True, desc='output file name')

out_figure = File(

'fd_power_2012.pdf', usedefault=True, desc='output figure name')

series_tr = traits.Float(desc='repetition time in sec.')

save_plot = traits.Bool(False, usedefault=True, desc='write FD plot')

normalize = traits.Bool(

False, usedefault=True, desc='calculate FD in mm/s')

figdpi = traits.Int(

100, usedefault=True, desc='output dpi for the FD plot')

figsize = traits.Tuple(

traits.Float(11.7),

traits.Float(2.3),

usedefault=True,

desc='output figure size')

I would suggest:

metric = traits.Enum('L1', 'SE3', usedefault=True, mandatory=True, desc='Distance metric to apply: ' 'L1 = Manhattan distance (original definition), 'SE3 = Special Euclidean group in 3D (geodesic)')

Or similar.

Let's go ahead and remove the metric='L1' parameter from _run_interface().

effigies · 2018-07-26T20:18:56Z

nipype/algorithms/confounds.py

-        diff[:, 3:6] *= self.inputs.radius
-        fd_res = np.abs(diff).sum(axis=1)
+
+        if metric == 'L1':


Then here (and below) you would use self.inputs.metric, instead of metric.

effigies · 2018-07-26T20:19:38Z

nipype/algorithms/confounds.py

+            diff[:, 3:6] *= self.inputs.radius
+            fd_res = np.abs(diff).sum(axis=1)
+
+        elif metric == 'riemannian':


I guess I said 'SE3' above. If 'riemannian' makes more sense, that's fine. I don't have strong opinions.

effigies · 2018-07-26T20:20:27Z

requirements.txt

+configparser
+funcsigs
+future>=0.16.0
+geomstats>=1.7


Can you please revert to the current master version of this file and only add geomstats>=1.7?

I wanted to sort them in alphabetical order to improve readability. But sure, I can revert! Is there any specific reason why we don't use alphabetical order there? Thanks.

Mostly just to make it obvious during PR review what's changed.

effigies · 2018-08-06T13:36:02Z

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.

ninamiolane · 2018-08-14T17:50:53Z

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!

nipype/nipype/utils/misc.py

Lines 227 to 238 in 4b2a821

    
           def normalize_mc_params(params, source): 
        
               """ 
        
               Normalize a single row of motion parameters to the SPM format. 
        
               SPM saves motion parameters as: 
        
                   x   Right-Left          (mm) 
        
                   y   Anterior-Posterior  (mm) 
        
                   z   Superior-Inferior   (mm) 
        
                   rx  Pitch               (rad) 
        
                   ry  Yaw                 (rad) 
        
                   rz  Roll                (rad) 
        
               """

…tance

effigies

Looks good. Just a couple mostly cosmetic changes. They can wait on hearing back from SPM.

effigies · 2018-08-16T18:45:36Z

nipype/algorithms/confounds.py

@@ -13,6 +13,7 @@

 import nibabel as nb
 import numpy as np
+


Just for the sake of keeping the diff clean, can you remove this newline?

effigies · 2018-08-16T18:47:56Z

nipype/algorithms/tests/test_auto_FramewiseDisplacement.py

@@ -12,6 +12,7 @@ def test_FramewiseDisplacement_inputs():
        out_figure=dict(usedefault=True, ),
        out_file=dict(usedefault=True, ),
        parameter_source=dict(mandatory=True, ),
+        metric=dict(usedefault=True, ),


Can you re-run make specs and re-commit test_auto_FramewiseDisplacement?

nipype/algorithms/tests/test_confounds.py

effigies · 2018-08-29T14:21:55Z

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.

effigies · 2018-09-17T13:29:33Z

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?

effigies · 2018-10-23T16:45:19Z

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

ninamiolane · 2018-10-23T20:59:16Z

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

effigies · 2018-11-15T14:56:47Z

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.

effigies · 2019-01-16T17:41:02Z

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.

…tance

Add geomstats module + geodesic distance for Frame Displacement

422fbdd

effigies reviewed Jun 4, 2018

View reviewed changes

nipype/algorithms/confounds.py Show resolved Hide resolved

ninamiolane added 3 commits June 6, 2018 16:12

Add geomstats module + geodesic distance for Frame Displacement

ea9f4dd

Add metric as an option to FramewiseDisplacement

9c65c57

Merge with remote branch

9a1cb9d

effigies reviewed Jul 26, 2018

View reviewed changes

effigies added this to the 1.1.2 milestone Jul 27, 2018

effigies modified the milestones: 1.1.2, 1.1.3 Aug 10, 2018

ninamiolane added 6 commits August 14, 2018 08:50

Add geomstats module + geodesic distance for Frame Displacement

30098a5

Add geomstats module + geodesic distance for Frame Displacement

b35fc5e

Add metric as an option to FramewiseDisplacement

6199667

Address Chris CR

8e7f2ee

Merge with origin nina-geodesic-distance

c74a833

Reverse requirements

308e622

ninamiolane and others added 5 commits August 14, 2018 10:52

Add conversion function from Euler angles

1e7e550

Put only use_default=True and add corresponding line in test auto

741f989

Add unit test for FrameDisplacement with Riemannian metric

8d1b6f6

Add geomstats in Dockerfile dependencies

8fb79c4

Merge remote-tracking branch 'upstream/master' into nina-geodesic-dis…

8f9ce0f

…tance

effigies reviewed Aug 16, 2018

View reviewed changes

ninamiolane added 3 commits August 28, 2018 14:06

Adapt convention for Euler angles from spm_matrix.m

0565df4

Address Chris CR

6089d27

Run specs for test_auto_FramewiseDisplacement.py

c82b5fb

effigies mentioned this pull request Sep 17, 2018

Split out dependencies into extras geomstats/geomstats#136

Closed

effigies mentioned this pull request Sep 19, 2018

REL: 1.1.3 #2706

Merged

10 tasks

effigies modified the milestones: 1.1.3, 1.1.4 Sep 21, 2018

effigies mentioned this pull request Oct 26, 2018

REL: 1.1.4 #2760

Merged

12 tasks

effigies modified the milestones: 1.1.4, 1.1.5 Oct 31, 2018

effigies modified the milestones: 1.1.5, 1.1.6 Nov 7, 2018

effigies modified the milestones: 1.1.6, 1.1.7 Nov 26, 2018

effigies modified the milestones: 1.1.7, 1.1.8 Dec 14, 2018

Merge branch 'master' into nina-geodesic-distance

c9a896c

effigies mentioned this pull request Jan 25, 2019

REL: 1.1.8 #2864

Merged

16 tasks

effigies modified the milestones: 1.1.8, future Jan 27, 2019

rciric mentioned this pull request Jan 28, 2019

[WIP][ENH] Confound model enhancement nipreps/fmriprep#1487

Closed

Merge remote-tracking branch 'upstream/master' into nina-geodesic-dis…

4177f4e

…tance

effigies closed this Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH WIP: Use geodesic distance in FramewiseDisplacement #2607

ENH WIP: Use geodesic distance in FramewiseDisplacement #2607

ninamiolane commented Jun 4, 2018

satra commented Jun 4, 2018

chrisgorgo commented Jun 4, 2018

codecov-io commented Jun 6, 2018 •

edited by codecov bot

Loading

effigies commented Jul 23, 2018

oesteban commented Jul 23, 2018

ninamiolane commented Jul 24, 2018

ninamiolane commented Jul 25, 2018 •

edited by effigies

Loading

effigies commented Jul 25, 2018

effigies left a comment

effigies Jul 26, 2018

effigies Aug 15, 2018

effigies Jul 26, 2018

effigies Jul 26, 2018

effigies Jul 26, 2018

ninamiolane Aug 14, 2018

effigies Aug 14, 2018

effigies commented Aug 6, 2018

ninamiolane commented Aug 14, 2018

effigies left a comment

effigies Aug 16, 2018

effigies Aug 16, 2018

effigies commented Aug 29, 2018

effigies commented Sep 17, 2018

effigies commented Oct 23, 2018

ninamiolane commented Oct 23, 2018

effigies commented Nov 15, 2018

effigies commented Jan 16, 2019

	class FramewiseDisplacementInputSpec(BaseInterfaceInputSpec):
	in_file = File(exists=True, mandatory=True, desc='motion parameters')
	parameter_source = traits.Enum(
	"FSL",
	"AFNI",
	"SPM",
	"FSFAST",
	"NIPY",
	desc="Source of movement parameters",
	mandatory=True)
	radius = traits.Float(
	50,
	usedefault=True,
	desc='radius in mm to calculate angular FDs, 50mm is the '
	'default since it is used in Power et al. 2012')
	out_file = File(
	'fd_power_2012.txt', usedefault=True, desc='output file name')
	out_figure = File(
	'fd_power_2012.pdf', usedefault=True, desc='output figure name')
	series_tr = traits.Float(desc='repetition time in sec.')
	save_plot = traits.Bool(False, usedefault=True, desc='write FD plot')
	normalize = traits.Bool(
	False, usedefault=True, desc='calculate FD in mm/s')
	figdpi = traits.Int(
	100, usedefault=True, desc='output dpi for the FD plot')
	figsize = traits.Tuple(
	traits.Float(11.7),
	traits.Float(2.3),
	usedefault=True,
	desc='output figure size')

ENH WIP: Use geodesic distance in FramewiseDisplacement #2607

ENH WIP: Use geodesic distance in FramewiseDisplacement #2607

Conversation

ninamiolane commented Jun 4, 2018

satra commented Jun 4, 2018

chrisgorgo commented Jun 4, 2018

codecov-io commented Jun 6, 2018 • edited by codecov bot Loading

Codecov Report

effigies commented Jul 23, 2018

oesteban commented Jul 23, 2018

ninamiolane commented Jul 24, 2018

ninamiolane commented Jul 25, 2018 • edited by effigies Loading

effigies commented Jul 25, 2018

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Aug 6, 2018

ninamiolane commented Aug 14, 2018

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Aug 29, 2018

effigies commented Sep 17, 2018

effigies commented Oct 23, 2018

ninamiolane commented Oct 23, 2018

effigies commented Nov 15, 2018

effigies commented Jan 16, 2019

codecov-io commented Jun 6, 2018 •

edited by codecov bot

Loading

ninamiolane commented Jul 25, 2018 •

edited by effigies

Loading