Add simulator #150

CameronTEllis · 2016-12-09T18:41:28Z

Updated simulator extensively

…sts and examples

…d TDA code.

…d_simulator Conflicts: setup.py

Conflicts: brainiak/utils/fmrisim.py examples/utils/fmrisim_example.py setup.py

mihaic · 2016-12-09T23:36:50Z

brainiak/utils/fmrisim.py

+    This estimates what noise properties the volume has. The absolute size of
+    the autoregression, drift and system noise is not critical but rather
+    the relative amounts (because all temporal noise is z scored in the
+    average and multiplied by the 'overall' parameter).


What is the overall parameter? Where is it (it is not a parameter of this function)?

It is a parameter used in generate_noise to control the overall noise level.

Could you please explain this in the docstring?

Okay updated

No need to make any change to hyperparamopt

mihaic · 2016-12-13T18:49:22Z

brainiak/utils/fmrisim.py

+from itertools import product
+import nibabel
 from scipy import stats
+import nitime.algorithms.autoregressive as ar


You must add Nitime to the list of dependencies in setup.py.

mihaic · 2016-12-13T18:52:14Z

brainiak/utils/fmrisim.py

+def export_stimfunction(stimfunction,
+                        filename,
+                        ):
+    """ Output a three column text file (onset time, event duration and weight)


Is this a tab-separated values file? If so, please change the docstring to say so.

mihaic · 2016-12-13T19:24:20Z

brainiak/utils/fmrisim.py

+    # nifti into a numpy array
+    if isinstance(volume, str):
+        nifti = nibabel.load(volume)  # Load the file
+        volume = nifti.get_data()  # Pull out the data


This is the only usage of Nibabel in your code (except for the example). Could you please remove it so we do not introduce a dependency on Nibabel? If so, please change the docstring and dependencies in setup.py accordingly.

I was told that Nibabel is the method we will be using for loading (and in this case writing it is well) in niftis. If there is an alternative, I am happy to use it.

It is indeed. However, so far we have only been using it in examples, not in BrainIAK code. I would prefer to not add it as a BrainIAK dependency yet, if it is not really necessary. The way I see it, here it is not necessary, because you can load the file outside BrainIAK, like you do now in the example.

Okay fair enough, I have removed it

mihaic · 2016-12-13T19:26:09Z

setup.py

        'numpy',
        'scikit-learn>=0.18',
        'scipy',
+        'statsmodels',


Not used. Please remove.

mihaic · 2016-12-13T19:28:26Z

tests/utils/test_fmrisim.py

-    assert np.sum(stimfunction) == np.sum(event_durations * len(onsets)) / \
-                                   tr_duration, "Event number"
+    assert np.sum(stimfunction) == np.sum(event_durations * len(onsets)) * \
+                                   1000, "Event number"


You should break the line before binary operators:
https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

mihaic · 2016-12-13T22:20:47Z

The testing error is because of a bug in Nitime:
nipy/nitime#141

Let's wait a couple of days for the Nitime maintainer to make a release including the bug fix before deciding how to proceed.

mihaic · 2016-12-16T04:27:29Z

Jenkins, retest this please.

mihaic · 2016-12-16T04:59:29Z

There is a new Nitime release which solves the installation problem.

@CameronTEllis, the failure is caused by low coverage now. Run pr-check.sh yourself followed by coverage report to see. (I will fix run-tests.sh to show the low coverage by itself.)

CameronTEllis · 2016-12-16T16:36:30Z

Hi Mihai, Thanks for following this up. My pr-check still isn't working (the coverage is not being run). How would you like me to proceed? In the meantime I am adding some some new tests now that should get my coverage up. Thanks, Cameron

…

On Thu, Dec 15, 2016 at 11:59 PM, Mihai Capotă ***@***.***> wrote: There is a new Nitime release which solves the installation problem. @CameronTEllis <https://github.com/CameronTEllis>, the failure is caused by low coverage now. Run pr-check.sh yourself followed by coverage report to see. (I will fix run-tests.sh to show the low coverage by itself.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#150 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKf6H72Z09XNW1T7QlqMxRUienQ-OTArks5rIhqygaJpZM4LJPAE> .

-- *Thank you,* *Cameron Ellis* *PhD Candidate (Turk-Browne lab) Department of Psychology Princeton University Peretsman-Scully Hall, Room 321 Princeton, NJ 08544 p: 609.258.7493*

mihaic · 2016-12-16T21:59:12Z

brainiak/utils/fmrisim.py

+
+        timing_file : string
+            The filename (with path) to a three column timing file (FSL) to
+            make the events. Still requires tr_duration and total_time


tr_duration is not a parameter of this function.

mihaic · 2016-12-16T22:02:18Z

brainiak/utils/fmrisim.py

                          total_time,
-                          tr_duration,
+                          weights=[1],
+                          timing_file=None,


You should add a paragraph to the docstring (before the parameters) explaining there are two ways to call the function: either with a timing file, or with onsets.

mihaic · 2016-12-16T22:33:10Z

brainiak/utils/fmrisim.py

+
+            # Pull out the weight, set as a float
+            weights[event_counter] = float(line[0:line.find('\t')])
+


This is correct, but not Pythonic. Consider something like this:

for line in f: onset, duration, weight = line.strip().split() onsets.append(float(onset)) durations.append(float(duration)) weights.append(float(weight))

mihaic · 2016-12-16T23:02:32Z

brainiak/utils/fmrisim.py

+    stimfunction = [0] * round(total_time * temporal_resolution)

    # Cycle through the onsets
    for onset_counter in list(range(0, len(onsets))):


No need for list and 0.

mihaic · 2016-12-16T23:07:39Z

brainiak/utils/fmrisim.py


    Apply the convolution of the HRF and stimulus time course to the
-    volume.
+    volume. This requires discriti


"discriti"?

mihaic · 2016-12-16T23:26:33Z

brainiak/utils/fmrisim.py

+        What is the path to the mask to be loaded? If set to self then it
+        makes a mask from the volume supplied (by averaging across time
+        points and changing the range). If empty then it defaults to an
+        MNI152 grey matter mask.


This gives a special meaning to the path "self". It would be better to introduce an extra boolean parameter, perhaps called self_mask.

mihaic · 2016-12-16T23:28:20Z

brainiak/utils/fmrisim.py

+    # tmp = str(noise_dict['physiological_sigma'] ** 2 / total_var * 100)[0:5]
+    # print('Physiological noise:\t' + tmp)
+    # tmp = str(noise_dict['system_sigma'] ** 2 / total_var * 100)[0:5]
+    # print('System noise:\t\t\t' + tmp)


Please remove or turn into debug log messages.

mihaic · 2016-12-16T23:40:34Z

brainiak/utils/fmrisim.py

+                   stimfunction,
+                   tr_duration,
+                   mask=None,
+                   noise_dict={'overall': 0.1},


The default value is only evaluated once and shared between all calls:
https://docs.python.org/3/tutorial/controlflow.html#default-argument-values
You should set None as the default value and make the assignment in the function body.

mihaic · 2016-12-16T23:44:10Z

brainiak/utils/fmrisim.py

+                                              )
+
+    noise_system = _generate_noise_system(dimensions_tr=dimensions_tr,
+                                          ) * noise_dict['system_sigma']


This style is appropriate for code that is expected to change. Do you expect to keep adding parameters to the _generate_noise_system call? Otherwise, consider this for readability:

noise_system = _generate_noise_system(dimensions_tr=dimensions_tr) * noise_dict['system_sigma']

mihaic · 2016-12-16T23:55:52Z

setup.py

    install_requires=[
        'cython',
        'mpi4py',
+        'nibabel',


Not used in BrainIAK. Move it to examples/utils/requirements.txt.

mihaic · 2017-01-09T21:20:35Z

@CameronTEllis, please speak up if you need help with the testing errors.

CameronTEllis · 2017-01-11T14:42:11Z

Hi Mihai, I am not sure what is meant by this. What errors are you referring to? Why did the most recent commit fail? Thanks, Cameron

…

On Tue, Jan 10, 2017 at 10:20 AM, Mihai Capotă ***@***.***> wrote: @CameronTEllis <https://github.com/CameronTEllis>, please speak up if you need help with the testing errors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#150 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKf6H1YVvJ4MPeWfhqZITbfagaBshLLoks5rQqSkgaJpZM4LJPAE> .

-- *Thank you,* *Cameron Ellis* *PhD Candidate (Turk-Browne lab) Department of Psychology Princeton University Peretsman-Scully Hall, Room 321 Princeton, NJ 08544 p: 609.258.7493*

mihaic · 2017-01-11T17:33:07Z

Click on the "Details" link next to the failed Travis build. Then click on any of the failed jobs to see the logs. Here is an the relevant part from at the end of one of the logs:

flake8 --config setup.cfg brainiak
brainiak/utils/fmrisim.py:721:62: E128 continuation line under-indented for visual indent
brainiak/utils/fmrisim.py:727:62: E128 continuation line under-indented for visual indent
brainiak/utils/fmrisim.py:733:62: E128 continuation line under-indented for visual indent
brainiak/utils/fmrisim.py:1320:80: E501 line too long (82 > 79 characters)
brainiak/utils/fmrisim.py:1507:20: E127 continuation line over-indented for visual indent
run-checks failed
The command "./pr-check.sh" exited with 1.
Done. Your build exited with 1.

If you run pr-check.sh on you own computer, you will get the same results.

mihaic · 2017-01-12T00:19:16Z

brainiak/utils/fmrisim.py:1506:20: E127 continuation line over-indented for visual indent
Try this:

    noise_system = (_generate_noise_system(dimensions_tr=dimensions_tr)
                    * noise_dict['system_sigma'])

Also, please merge the latest changes from master. You can also do it on github.com using the "Update branch" button after you push all your changes.

mihaic · 2017-01-19T01:07:56Z

Jenkins, retest this please.

mihaic · 2017-01-19T02:04:07Z

@CameronTEllis, there were other errors unrelated to your code that were fixed. Please merge the latest commits from master (you can press "Update branch" in the PR on github.com and then pull the changes to your local repository).

However, there is also a problem with your code, which you can see when running the tests:

=================================== FAILURES ===================================
_______________________________ test_calc_noise ________________________________

    def test_calc_noise():
    
        # Inputs for functions
        onsets = [10, 30, 50, 70, 90]
        event_durations = [6]
        tr_duration = 2
        duration = 100
        dimensions_tr = np.array([10, 10, 10, duration/tr_duration])
    
        # Preset the noise dict
        nd_orig = {'auto_reg_sigma': 1,
                   'drift_sigma': 0.5,
                   'overall': 0.1,
                   'snr': 30,
                   'spatial_sigma': 0.15,
                   'system_sigma': 1,
                   }
    
        # Create the time course for the signal to be generated
        stimfunction = sim.generate_stimfunction(onsets=onsets,
                                                 event_durations=event_durations,
                                                 total_time=duration,
                                                 )
    
        # Mask the volume to be the same shape as a brain
>       mask = sim.mask_brain(dimensions_tr)

tests/utils/test_fmrisim.py:286: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
brainiak/utils/fmrisim.py:1338: in mask_brain
    volume = np.ones(volume)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

shape = array([ 10.,  10.,  10.,  50.]), dtype = None, order = 'C'

    def ones(shape, dtype=None, order='C'):
        """
        Return a new array of given shape and type, filled with ones.
    
        Parameters
        ----------
        shape : int or sequence of ints
            Shape of the new array, e.g., ``(2, 3)`` or ``2``.
        dtype : data-type, optional
            The desired data-type for the array, e.g., `numpy.int8`.  Default is
            `numpy.float64`.
        order : {'C', 'F'}, optional
            Whether to store multidimensional data in C- or Fortran-contiguous
            (row- or column-wise) order in memory.
    
        Returns
        -------
        out : ndarray
            Array of ones with the given shape, dtype, and order.
    
        See Also
        --------
        zeros, ones_like
    
        Examples
        --------
        >>> np.ones(5)
        array([ 1.,  1.,  1.,  1.,  1.])
    
        >>> np.ones((5,), dtype=np.int)
        array([1, 1, 1, 1, 1])
    
        >>> np.ones((2, 1))
        array([[ 1.],
               [ 1.]])
    
        >>> s = (2,2)
        >>> np.ones(s)
        array([[ 1.,  1.],
               [ 1.,  1.]])
    
        """
>       a = empty(shape, dtype, order)
E       TypeError: 'numpy.float64' object cannot be interpreted as an integer

../../.local/lib/python3.5/site-packages/numpy/core/numeric.py:192: TypeError
====================== 1 failed, 5 passed in 2.51 seconds ======================

Specifically, dimensions_tr = np.array([10, 10, 10, duration/tr_duration]) can contain a non-integer in the last position, thereby causing the error. Please fix this.

…nIAK into add_simulator

mihaic · 2017-01-20T00:57:06Z

Thank you for the contribution, @CameronTEllis.

I suggest you start a new branch for master for your next contribution. You should delete this branch; you can press the "Delete branch" button at the end of the PR page on github.com (you still need to delete it from your local repository).

#150) * Experimental utils for copying directories to other machines in the cluster using Ray. * Test copying directory functionality. * Small fix.

CameronTEllis and others added 18 commits July 19, 2016 22:15

Add TDA initial script.

d4dc002

Merge branch 'master' into add_tda

90fb540

Added documentation

42cb2b4

Added documentation

11d69a2

Merge branch 'master' into add_tda

e8eccc3

Added documentation

155de9a

Added the test_tda

fdd7f2b

Simulator initial commit. Includes the simulator function with the te…

75ae3d8

…sts and examples

Error in uploading simulator

54c00c7

Updated simulator based on edits. Added two condition example. Remove…

3099939

…d TDA code.

Merge branch 'master' of https://github.com/IntelPNI/BrainIAK into ad…

3222e3b

…d_simulator Conflicts: setup.py

Updated names, improved comments, changed load procedure for mask

709baae

Updated simulator

32ca9c8

Merge https://github.com/IntelPNI/BrainIAK into add_simulator

071eb7b

Conflicts: brainiak/utils/fmrisim.py examples/utils/fmrisim_example.py setup.py

Updated the way TDA features are generated

f0223ec

Updated tests for new simulator code

e26bfa9

Updated fmrisim test, corrected error of deleting test_hpo

1f9de4e

No need to make any change to hyperparamopt

25929fa

mihaic reviewed Dec 9, 2016

View reviewed changes

CameronTEllis added 2 commits December 13, 2016 13:15

Updated doc string

65ebfaa

Merge pull request #1 from mihaic/add_simulator

0ed711e

No need to make any change to hyperparamopt

mihaic requested changes Dec 13, 2016

View reviewed changes

CameronTEllis added 2 commits December 13, 2016 15:30

Conforming to PEP8

88961c8

Removed nibabel

8bfa0c5

Increased test coverage

f2c2c60

mihaic requested changes Dec 16, 2016

View reviewed changes

Updated for PEP8

7a04a24

Updated for pep8

c60a98b

CameronTEllis added 4 commits January 12, 2017 09:10

Updated for pep8

1ab5d9f

Updated pep8

63514bf

Edited the tests

0614cca

Complexity tests updating

4403f1e

CameronTEllis added 3 commits January 19, 2017 18:43

Merge branch 'master' into add_simulator

cafb3f7

Updated tests

77f736f

Merge branch 'add_simulator' of https://github.com/CameronTEllis/Brai…

b1ceaca

…nIAK into add_simulator

mihaic approved these changes Jan 20, 2017

View reviewed changes

mihaic merged commit 1c37d96 into brainiak:master Jan 20, 2017

danielsuo pushed a commit that referenced this pull request Nov 16, 2017

log duration of tasks (#150)

160bd6e


		# Pull out the weight, set as a float
		weights[event_counter] = float(line[0:line.find('\t')])

Add simulator #150

Add simulator #150

Uh oh!

Conversation

CameronTEllis commented Dec 9, 2016

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!

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!

mihaic commented Dec 13, 2016

Uh oh!

mihaic commented Dec 16, 2016

Uh oh!

mihaic commented Dec 16, 2016

Uh oh!

CameronTEllis commented Dec 16, 2016 via email

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!

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!

mihaic commented Jan 9, 2017

Uh oh!

CameronTEllis commented Jan 11, 2017 via email

Uh oh!

mihaic commented Jan 11, 2017

Uh oh!

mihaic commented Jan 12, 2017

Uh oh!

mihaic commented Jan 19, 2017

Uh oh!

mihaic commented Jan 19, 2017

Uh oh!

mihaic commented Jan 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants