ENH: Add an activation count map interface #2522

oesteban · 2018-03-30T22:54:08Z

A simple calculation of ACMs, adapted from @jokedurnez's code: https://github.com/poldracklab/CNP_task_analysis/blob/61c27f5992db9d8800884f8ffceb73e6957db8af/CNP_2nd_level_ACM.py

effigies · 2018-03-30T23:47:48Z

nipype/algorithms/stats.py

+
+    def _run_interface(self, runtime):
+        allmaps = nb.concat_images(
+            [nb.load(f) for f in self.inputs.in_files])


I assume you want to .get_data() somewhere here, before making numeric comparisons.

effigies

Looks good. One comment.

Also make specs.

effigies · 2018-03-31T14:48:08Z

nipype/algorithms/stats.py

+        allmaps = nb.concat_images(
+            [nb.load(f) for f in self.inputs.in_files]).get_data()
+        acm_pos = (allmaps > self.inputs.threshold).astype(
+            float).mean(3)


What about:

acm_pos = np.mean(allmaps > self.inputs.threshold, axis=3, dtype=np.float32)

I think it reads more cleanly, and it'll make any line breaks cleaner looking, too. (I also prefer to use explicit axis and dtype keywords, just to avoid having to look up the order when I read. And I prefer to use actual dtypes, just to avoid depending on numpy magic that they can change at any time.)

…ount-map

effigies · 2018-04-01T02:07:06Z

nipype/algorithms/stats.py

+
+    def _run_interface(self, runtime):
+        allmaps = nb.concat_images(
+            [nb.load(f) for f in self.inputs.in_files]).get_data()


Just realizing you can drop the load step here. concat_images will take file names.

effigies · 2018-04-01T02:26:23Z

LGTM. Have you tested it with some data, just to be sure?

effigies · 2018-04-01T02:30:12Z

I guess, really, we could write a test that randomly generates some small (e.g. 10x10x10) images and runs the interface. Shouldn't add significant run time. Your call if it seems worth the effort.

oesteban · 2018-04-01T03:03:26Z

I'll add the test 👍

…e into enh/activations-count-map

Pending test

effigies

Had another look at this, and got to thinking that we could probably make it a little friendlier to non-NIFTI-1 file formats, which should help preserve header information. Two comments are suggestions on how to achieve this.

Also, I think a longer name might be a good idea, here.

effigies · 2018-04-10T13:56:37Z

nipype/algorithms/stats.py

+        self._results['acm_pos'] = os.path.join(
+            runtime.cwd, 'acm_pos.nii.gz')
+        self._results['acm_neg'] = os.path.join(
+            runtime.cwd, 'acm_neg.nii.gz')


Maybe let's make this:

template_fname = self.inputs.in_files[0] ext = split_filename(template_fname)[2] fname_fmt = os.path.join(runtime.cwd, 'acm_{}' + ext).format self._results['out_file'] = fname_fmt('diff') self._results['acm_pos'] = fname_fmt('pos') self._results['acm_neg'] = fname_fmt('neg') img = nb.load(template_fname)

effigies · 2018-04-10T13:57:52Z

nipype/algorithms/stats.py

+        nb.Nifti1Image(
+            acm_pos, nii.affine, nii.header).to_filename(self._results['acm_pos'])
+        nb.Nifti1Image(
+            acm_neg, nii.affine, nii.header).to_filename(self._results['acm_neg'])


And then we would make these:

img.__class__(acm_diff, img.affine, img.header).to_filename(self._results['out_file']) img.__class__(acm_pos, img.affine, img.header).to_filename(self._results['acm_pos']) img.__class__(acm_neg, img.affine, img.header).to_filename(self._results['acm_neg'])

effigies · 2018-04-10T13:58:29Z

nipype/algorithms/stats.py

+    acm_neg = File(exists=True, desc='negative activation count map')
+
+
+class ACM(SimpleInterface):


I think I'd prefer a more descriptive name like ActivationCount.

effigies · 2018-04-10T14:05:05Z

See also: oesteban#7

TEST: Activation count map smoke test

oesteban · 2018-04-10T20:58:17Z

Some second opinion about this addition to nipype? @djarecka, @mgxd, @satra or @chrisfilo ?

Maybe a quick double check on the correctness from @jokedurnez ?

djarecka · 2018-04-13T14:41:16Z

nipype/algorithms/stats.py

+    threshold = traits.Float(1.65, usedefault=True,
+                             desc='binarization threshold. A z-value of 1.65 '
+                                  'corresponds to a two-sided test of p<.10')
+


I'm wondering if it's better to leave threshold without default value, so users have to really choose one....

I don't have an opinion here.

Makes sense, I'll update accordingly.

djarecka · 2018-04-13T14:52:05Z

nipype/algorithms/tests/test_stats.py

+    diff = nb.load(res.outputs.out_file)
+    pos = nb.load(res.outputs.acm_pos)
+    neg = nb.load(res.outputs.acm_neg)
+    assert np.allclose(diff.get_data(), pos.get_data() - neg.get_data())


we could also check some statistics pos.get_data itself?

The main things I can think of that should be true of performing this on some random noise would be np.all(pos.get_data() > 0) and np.all(neg.get_data() > 0).

If I understand correctly, the interface just checks how many point are above threshold or below -threshold. If we draw points from random distribution and we slightly increase the size of the array, we should be able to compare to theoretical value.
Let me finish something and I'll add a simple example to show what do I mean.

here is an example: oesteban@6e53f01

That looks valid, but I'm not fully clear on what assurances this gets us for the interface. I guess, roughly, that Gaussian noise is being filtered as we'd expect?

I'm okay if this test goes in, but my overall feeling is that a useful test asserts properties that should be true given any real or simulated inputs, and this seems tied to the specific distribution of simulated random variates.

you usually use specific cases for tests, and often some simple cases, so you know the answer.

The interface should work for any real or simulated inputs, but I'm checking here for a specific distribution (exactly the same as @oesteban used in the original test). Obviously I'm not able to predict answers for completely random set of numbers, but for the normal distribution I can.

I just thought that this adds extra checks to the original @oesteban test, that was only checking pos-neg == diff, but I won't insist to include it.

we can also just generate a small array and calculate the expected output of the interface.

…an/nipype into oesteban-enh/activations-count-map

effigies · 2018-04-25T13:27:27Z

@oesteban I think you said you're going to drop the threshold. Anything else before merging?

Oesteban enh/activations count map

…ount-map

…e into enh/activations-count-map

oesteban · 2018-04-25T17:56:10Z

Provided that the tests pass, this should be good to go.

effigies · 2018-04-25T17:58:09Z

nipype/algorithms/stats.py

+                              desc='input file, generally a list of z-stat maps')
+    threshold = traits.Float(
+        mandatory=True, desc='binarization threshold. E.g. a threshold of 1.65 '
+                             'corresponds to a two-sided test of p<.10')


"two-sided Z-test"?

[ENH] Add a activation count map interface

56fbac3

oesteban requested a review from effigies March 30, 2018 22:54

add credit to @jokedurnez [skip ci]

f6a9a81

effigies reviewed Mar 30, 2018

View reviewed changes

fix several issues

756088a

effigies previously approved these changes Mar 31, 2018

View reviewed changes

oesteban added 3 commits March 31, 2018 18:39

Merge remote-tracking branch 'upstream/master' into enh/activations-c…

4cf7f66

…ount-map

cleaner reading

81167ca

make specs

8a294e0

effigies reviewed Apr 1, 2018

View reviewed changes

drop nb.load

068dda0

oesteban changed the title ~~[ENH] Add a activation count map interface~~ [ENH] Add an activation count map interface Apr 1, 2018

oesteban changed the title ~~[ENH] Add an activation count map interface~~ ENH: Add an activation count map interface Apr 1, 2018

Merge branch 'enh/activations-count-map' of github.com:oesteban/nipyp…

5b9fc04

…e into enh/activations-count-map

effigies added this to the 1.0.3 milestone Apr 1, 2018

TEST: Activation count map smoke test

beaf3b8

effigies reviewed Apr 10, 2018

View reviewed changes

oesteban added 4 commits April 10, 2018 09:06

address @effigies' comments

4b93c9e

Merge pull request #7 from effigies/enh/activations-count-map

60e97d5

TEST: Activation count map smoke test

update test and autotest

608c4d2

remove old autotest

f81c1cf

effigies approved these changes Apr 10, 2018

View reviewed changes

djarecka reviewed Apr 13, 2018

View reviewed changes

djarecka added 3 commits April 13, 2018 11:11

Merge branch 'enh/activations-count-map' of https://github.com/oesteb…

b126b70

…an/nipype into oesteban-enh/activations-count-map

adding tests that checks pos/neg for normal distribution

6e53f01

increasing atol in the test

6348862

oesteban added 4 commits April 25, 2018 10:51

Merge pull request #9 from djarecka/oesteban-enh/activations-count-map

9e07a6e

Oesteban enh/activations count map

Merge remote-tracking branch 'upstream/master' into enh/activations-c…

d3d75f7

…ount-map

make threshold mandatory without default

e0ba996

Merge branch 'enh/activations-count-map' of github.com:oesteban/nipyp…

02b7dfd

…e into enh/activations-count-map

effigies reviewed Apr 25, 2018

View reviewed changes

oesteban added 3 commits April 25, 2018 10:59

forgot make specs

cfb19c4

[skip ci] improve description

31bc311

fix test

728bc1b

effigies merged commit a2d0f57 into nipy:master Apr 25, 2018

oesteban deleted the enh/activations-count-map branch October 26, 2018 06:28

		acm_neg = File(exists=True, desc='negative activation count map')


		class ACM(SimpleInterface):

ENH: Add an activation count map interface #2522

ENH: Add an activation count map interface #2522

Uh oh!

Conversation

oesteban commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Apr 1, 2018

Uh oh!

effigies commented Apr 1, 2018

Uh oh!

oesteban commented Apr 1, 2018

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Apr 10, 2018

Uh oh!

oesteban commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djarecka Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djarecka Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Apr 25, 2018

Uh oh!

oesteban commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

djarecka Apr 13, 2018 •

edited

Loading

djarecka Apr 17, 2018 •

edited

Loading