Skip to content

ENH WIP: Use geodesic distance in FramewiseDisplacement #2607

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.

Sign up for GitHub

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

Closed
wants to merge 21 commits into from
Closed

ENH WIP: Use geodesic distance in FramewiseDisplacement #2607

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
422fbdd
Add geomstats module + geodesic distance for Frame Displacement
ninamiolane Jun 4, 2018
ea9f4dd
Add geomstats module + geodesic distance for Frame Displacement
ninamiolane Jun 4, 2018
9c65c57
Add metric as an option to FramewiseDisplacement
ninamiolane Jun 6, 2018
9a1cb9d
Merge with remote branch
ninamiolane Jun 6, 2018
30098a5
Add geomstats module + geodesic distance for Frame Displacement
ninamiolane Jun 4, 2018
b35fc5e
Add geomstats module + geodesic distance for Frame Displacement
ninamiolane Jun 4, 2018
6199667
Add metric as an option to FramewiseDisplacement
ninamiolane Jun 6, 2018
8e7f2ee
Address Chris CR
ninamiolane Aug 14, 2018
c74a833
Merge with origin nina-geodesic-distance
ninamiolane Aug 14, 2018
308e622
Reverse requirements
ninamiolane Aug 14, 2018
1e7e550
Add conversion function from Euler angles
ninamiolane Aug 14, 2018
741f989
Put only use_default=True and add corresponding line in test auto
ninamiolane Aug 14, 2018
8d1b6f6
Add unit test for FrameDisplacement with Riemannian metric
ninamiolane Aug 14, 2018
8fb79c4
Add geomstats in Dockerfile dependencies
ninamiolane Aug 15, 2018
8f9ce0f
Merge remote-tracking branch 'upstream/master' into nina-geodesic-dis…
effigies Aug 15, 2018
5856288
Address Chris CR
ninamiolane Aug 15, 2018
0565df4
Adapt convention for Euler angles from spm_matrix.m
ninamiolane Aug 28, 2018
6089d27
Address Chris CR
ninamiolane Aug 28, 2018
c82b5fb
Run specs for test_auto_FramewiseDisplacement.py
ninamiolane Aug 28, 2018
c9a896c
Merge branch 'master' into nina-geodesic-distance
effigies Jan 16, 2019
4177f4e
Merge remote-tracking branch 'upstream/master' into nina-geodesic-dis…
effigies Feb 24, 2020
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions nipype/algorithms/confounds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@

import nibabel as nb
import numpy as np

Copy link
Member

Choose a reason for hiding this comment

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

Just for the sake of keeping the diff clean, can you remove this newline?
from geomstats.invariant_metric import InvariantMetric
from geomstats.special_euclidean_group import SpecialEuclideanGroup
from numpy.polynomial import Legendre
from scipy import linalg

Expand All @@ -26,6 +29,10 @@

IFLOGGER = logging.getLogger('interface')

SE3_GROUP = SpecialEuclideanGroup(n=3)
DIM_TRANSLATIONS = SE3_GROUP.translations.dimension
DIM_ROTATIONS = SE3_GROUP.rotations.dimension
Copy link
Member

Choose a reason for hiding this comment

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

There are three of these, probably due to some merging quirks. But if we're going to import geomstats within the interface, we'll actually want to get rid of these altogether, here.


class ComputeDVARSInputSpec(BaseInterfaceInputSpec):
in_file = File(
Expand Down Expand Up @@ -300,16 +307,37 @@ class FramewiseDisplacement(BaseInterface):
'tags': ['method']
}]

def _run_interface(self, runtime):
def _run_interface(self, runtime, metric='L1'):
Copy link
Member

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

Let's go ahead and remove the metric='L1' parameter from _run_interface().
mpars = np.loadtxt(self.inputs.in_file) # mpars is N_t x 6
mpars = np.apply_along_axis(
func1d=normalize_mc_params,
axis=1,
arr=mpars,
source=self.inputs.parameter_source)
diff = mpars[:-1, :6] - mpars[1:, :6]
diff[:, 3:6] *= self.inputs.radius
fd_res = np.abs(diff).sum(axis=1)

if metric == 'L1':
Copy link
Member

Choose a reason for hiding this comment

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

Then here (and below) you would use self.inputs.metric, instead of metric.
diff = mpars[:-1, :6] - mpars[1:, :6]
diff[:, 3:6] *= self.inputs.radius
fd_res = np.abs(diff).sum(axis=1)

elif metric == 'riemannian':
Copy link
Member

Choose a reason for hiding this comment

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

I guess I said 'SE3' above. If 'riemannian' makes more sense, that's fine. I don't have strong opinions.
# TODO(nina): convert to geomstats parameterization:
se3pars = np.hstack(
[mpars[:, DIM_TRANSLATIONS:], mpars[:, :DIM_TRANSLATIONS]])

diag_rotations = self.inputs.radius * np.ones(DIM_ROTATIONS)
diag_translations = np.ones(DIM_TRANSLATIONS)
diag = np.concatenate([diag_rotations, diag_translations])
inner_product = np.diag(diag)
metric = InvariantMetric(
group=SE3_GROUP,
inner_product_mat_at_identity=inner_product,
left_or_right='left')
fd_res = metric.dist(se3pars[:-1], se3pars[1:])

else:
raise ValueError(
'The parameter \'metric\' should be \'L1\' or \'riemannian\'.')
Copy link
Member

Choose a reason for hiding this comment

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

By using the Enum trait with mandatory=True above, the only possible values are 'L1' or 'riemannian', so we can drop this final else.

self._results = {
'out_file': op.abspath(self.inputs.out_file),
Expand Down
27 changes: 14 additions & 13 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
numpy>=1.9.0
scipy>=0.14
click>=6.6.0
configparser
funcsigs
future>=0.16.0
geomstats>=1.7
Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

Mostly just to make it obvious during PR review what's changed.
mock
networkx>=1.9
traits>=4.6
python-dateutil>=2.2
nibabel>=2.1.0
future>=0.16.0
simplejson>=3.8.0
numpy>=1.9.0
python-dateutil>=2.2
packaging
prov==1.5.0
click>=6.6.0
funcsigs
configparser
pydotplus
pydot>=1.2.3
pytest>=3.0
pytest-xdist
pytest-env
mock
pydotplus
pydot>=1.2.3
packaging
scipy>=0.14
simplejson>=3.8.0
traits>=4.6