-
Notifications
You must be signed in to change notification settings - Fork 532
[ENH] CompCor enhancement #2878
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.
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
Changes from 38 commits
329c74d
17f3e12
114e6d4
6f4fc19
4d2208e
bfbde82
0373879
2c551d0
a53cd46
b811d47
2743189
577e395
66c7540
0bb0096
94bea4a
addb0e9
b04c9ca
e957e87
797801e
67a3276
fe430f5
1625bdb
9afb3f5
689d064
f390bc6
ad3d440
fd41b74
01a78ec
a742c9c
518a489
deceb95
fa64907
e6dfe7d
27ed03f
422c04c
144fca3
82a25c2
4c1af8a
79e840d
1b1b6fa
89ba3b4
b80a3d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||||
# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*- | ||||||
# vi: set ft=python sts=4 ts=4 sw=4 et: | ||||||
import os | ||||||
import re | ||||||
|
||||||
import nibabel as nb | ||||||
import numpy as np | ||||||
|
@@ -48,20 +49,47 @@ def test_compcor(self): | |||||
|
||||||
self.run_cc( | ||||||
CompCor( | ||||||
num_components=6, | ||||||
realigned_file=self.realigned_file, | ||||||
mask_files=self.mask_files, | ||||||
mask_index=0), expected_components) | ||||||
|
||||||
self.run_cc( | ||||||
ACompCor( | ||||||
num_components=6, | ||||||
realigned_file=self.realigned_file, | ||||||
mask_files=self.mask_files, | ||||||
mask_index=0, | ||||||
components_file='acc_components_file'), expected_components, | ||||||
'aCompCor') | ||||||
|
||||||
def test_compcor_variance_threshold_and_metadata(self): | ||||||
expected_components = [['-0.2027150345', '-0.4954813834'], | ||||||
['0.2565929051', '0.7866217875'], | ||||||
['-0.3550986008', '-0.0089784905'], | ||||||
['0.7512786244', '-0.3599828482'], | ||||||
['-0.4500578942', '0.0778209345']] | ||||||
expected_metadata = { | ||||||
'component': 'CompCor00', | ||||||
'mask': 'mask', | ||||||
'singular_value': '4.0720553036', | ||||||
'variance_explained': '0.5527211465', | ||||||
'cumulative_variance_explained': '0.5527211465' | ||||||
} | ||||||
ccinterface = CompCor( | ||||||
variance_threshold=0.7, | ||||||
realigned_file=self.realigned_file, | ||||||
mask_files=self.mask_files, | ||||||
mask_names=['mask'], | ||||||
mask_index=1, | ||||||
save_metadata=True) | ||||||
self.run_cc(ccinterface=ccinterface, | ||||||
expected_components=expected_components, | ||||||
expected_n_components=2, | ||||||
expected_metadata=expected_metadata) | ||||||
|
||||||
def test_tcompcor(self): | ||||||
ccinterface = TCompCor( | ||||||
ccinterface = TCompCor(num_components=6, | ||||||
realigned_file=self.realigned_file, percentile_threshold=0.75) | ||||||
self.run_cc(ccinterface, [['-0.1114536190', '-0.4632908609'], [ | ||||||
'0.4566907310', '0.6983205193' | ||||||
|
@@ -70,7 +98,8 @@ def test_tcompcor(self): | |||||
], ['-0.1342351356', '0.1407855119']], 'tCompCor') | ||||||
|
||||||
def test_tcompcor_no_percentile(self): | ||||||
ccinterface = TCompCor(realigned_file=self.realigned_file) | ||||||
ccinterface = TCompCor(num_components=6, | ||||||
realigned_file=self.realigned_file) | ||||||
ccinterface.run() | ||||||
|
||||||
mask = nb.load('mask_000.nii.gz').get_data() | ||||||
|
@@ -80,6 +109,7 @@ def test_tcompcor_no_percentile(self): | |||||
def test_compcor_no_regress_poly(self): | ||||||
self.run_cc( | ||||||
CompCor( | ||||||
num_components=6, | ||||||
realigned_file=self.realigned_file, | ||||||
mask_files=self.mask_files, | ||||||
mask_index=0, | ||||||
|
@@ -151,7 +181,9 @@ def test_tcompcor_multi_mask_no_index(self): | |||||
def run_cc(self, | ||||||
ccinterface, | ||||||
expected_components, | ||||||
expected_header='CompCor'): | ||||||
expected_header='CompCor', | ||||||
expected_n_components=None, | ||||||
expected_metadata=None): | ||||||
# run | ||||||
ccresult = ccinterface.run() | ||||||
|
||||||
|
@@ -160,13 +192,14 @@ def run_cc(self, | |||||
assert ccresult.outputs.components_file == expected_file | ||||||
assert os.path.exists(expected_file) | ||||||
assert os.path.getsize(expected_file) > 0 | ||||||
assert ccinterface.inputs.num_components == 6 | ||||||
|
||||||
with open(ccresult.outputs.components_file, 'r') as components_file: | ||||||
expected_n_components = min(ccinterface.inputs.num_components, | ||||||
self.fake_data.shape[3]) | ||||||
if expected_n_components is None: | ||||||
expected_n_components = min(ccinterface.inputs.num_components, | ||||||
self.fake_data.shape[3]) | ||||||
|
||||||
components_data = [line.split('\t') for line in components_file] | ||||||
components_data = [re.sub('\n', '', line).split('\t') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just stripping the newline? If so,
Suggested change
|
||||||
for line in components_file] | ||||||
|
||||||
# the first item will be '#', we can throw it out | ||||||
header = components_data.pop(0) | ||||||
|
@@ -180,9 +213,24 @@ def run_cc(self, | |||||
num_got_timepoints = len(components_data) | ||||||
assert num_got_timepoints == self.fake_data.shape[3] | ||||||
for index, timepoint in enumerate(components_data): | ||||||
assert (len(timepoint) == ccinterface.inputs.num_components | ||||||
or len(timepoint) == self.fake_data.shape[3]) | ||||||
assert (len(timepoint) == expected_n_components) | ||||||
assert timepoint[:2] == expected_components[index] | ||||||
|
||||||
if ccinterface.inputs.save_metadata: | ||||||
expected_metadata_file = ( | ||||||
ccinterface._list_outputs()['metadata_file']) | ||||||
assert ccresult.outputs.metadata_file == expected_metadata_file | ||||||
assert os.path.exists(expected_metadata_file) | ||||||
assert os.path.getsize(expected_metadata_file) > 0 | ||||||
|
||||||
with open(ccresult.outputs.metadata_file, 'r') as metadata_file: | ||||||
components_metadata = [re.sub('\n', '', line).split('\t') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, if I understand the purpose:
Suggested change
|
||||||
for line in metadata_file] | ||||||
components_metadata = {i: j for i, j in | ||||||
zip(components_metadata[0], | ||||||
components_metadata[1])} | ||||||
assert components_metadata == expected_metadata | ||||||
|
||||||
return ccresult | ||||||
|
||||||
@staticmethod | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -141,7 +141,8 @@ def get_nipype_gitversion(): | |||||
'numpy>=%s ; python_version >= "3.7"' % NUMPY_MIN_VERSION_37, | ||||||
'python-dateutil>=%s' % DATEUTIL_MIN_VERSION, | ||||||
'scipy>=%s' % SCIPY_MIN_VERSION, | ||||||
'traits>=%s' % TRAITS_MIN_VERSION, | ||||||
'traits>=%s,<%s ; python_version == "2.7"' % (TRAITS_MIN_VERSION, '5.0.0'), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is #2913. The fix should be just to skip 5.0, and we can do that for all versions.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with this, though we should probably only restrict 5.0 if the user is running py27 - to my knowledge everything was working fine with py3. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a strong opinion here (this seems simpler than splitting out by Python version, but yes there may be someone who really needs Py3 + traits 5.0), but I think maybe let's make that change after this is merged? |
||||||
'traits>=%s ; python_version >= "3.0"' % TRAITS_MIN_VERSION, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
'future>=%s' % FUTURE_MIN_VERSION, | ||||||
'simplejson>=%s' % SIMPLEJSON_MIN_VERSION, | ||||||
'prov>=%s' % PROV_VERSION, | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggestions below were prompted by a suspicion this might be heavier than needed. If you accept those changes, also revert: