Random permutation of FCMA by yidawang · Pull Request #217 · brainiak/brainiak

yidawang · 2017-04-27T05:29:44Z

add random voxel permutation to FCMA;
add more debugging and information logs;
minor tweak for docstring format, etc.

Thanks in advance for reviewing.

xoltar

Thanks, Yida! Looks pretty good, just a few small changes needed.

xoltar · 2017-04-27T16:03:14Z

            raw_data2, _ = _separate_epochs(activity_data2, conditions)
        else:
            activity_data1 = list(mask_images(images, mask1, np.float32))
+        _randomize_subject_list(activity_data1, random)


_randomize_single_subject modifies data in place (and then returns the same array). _randomize_subject_list returns a new list, containing old arrays that have been modified in place, which is why these lines work even though you're ignoring the return value. I would say either write them both to work in-place, or write them both to return new values. Mixing the two makes for confusion. If you decide you want to have _randomize_single_subject return a new array instead of working in-place, consider numpy.random.permutation instead of shuffle.

Thanks for the suggestion! Somehow I ignored the return value here although I should put it as activity_data1 = _randomize_subject_list(activity_data1, random). I would prefer working in-place, let me modify _randomize_subject_list accordingly.

xoltar · 2017-04-27T16:03:38Z

+
+    Parameters
+    ----------
+    data: 2D array in shape [nVxels, nTRs]


xoltar · 2017-04-27T16:03:52Z

+        Seed for random state used implicitly for shuffling.
+
+    Returns
+    data: 2D array in shape [nVxels, nTRs]


xoltar · 2017-04-27T16:06:51Z

        data = f.get_data().astype(data_type)
+        [d1, d2, d3, d4] = data.shape
+        if random == RandomType.REPRODUCIBLE:
+            data = _randomize_single_subject(data.reshape((d1 * d2 * d3, d4)),


You could make this reshaping logic part of _randomize_single_subject (i.e. have it flatten, randomize, then reshape into whatever shape the original array was).

I didn't do this because I would like to make the input argument data of _randomize_single_subject an nVoxels x nTRs 2D array for clarity purpose.

xoltar · 2017-04-27T16:08:14Z


-        logger.info(
+        logger.debug(
            'file %s is loaded and processed, with data shape %s' %


logger does formatting itself, you can just do:
logger.debug('file %s is loaded and processed, with data shape $s', f.get_filename(), data.shape)

xoltar · 2017-04-27T16:09:17Z

        """
        logger.info(
-            'Master starts to allocate tasks'
+            'Master at rank %d starts to allocate tasks' %


Same logging comments apply to this file.

xoltar · 2017-04-27T16:09:52Z

    for f in files:
        yield nib.load(str(f))
+        logger.info(
+            'file %s is read' %


Here too. No need for str(), either.

f is a path here, which is not supported to be the argument of numpy.load(), and presumably nibabel.load(), for numpy version before 1.12. @mihaic should be able to further justify this.

Right. However, Paths work with string formatting without explicit casting to str, e.g.:
"test %s" % pathlib.Path("/")

OK, just realized that Bryn was referring the str usage below this line :) Done.

mihaic · 2017-04-27T16:51:34Z

        Type to cast image to.
+    random: Optional[RandomType]
+        Randomize the data within subject or not.
+        Default NORANDOM


You should not specify the default because you are duplicating the information in the function definition, which is visible both in the code and in the generated docs. (Of course, if you use None as a placeholder, you should explain what it stands for.)

mihaic · 2017-04-27T16:58:14Z


 import nibabel as nib
 import numpy as np
+import logging


Use alphabetical order, please. I will introduce a checker real soon now®.

mihaic · 2017-04-27T17:04:14Z

+        logger.info(
+            'file %s is read',
+            f
+        )


I would say you should use the debug level for the messages you added in "io.py".

Also, do not break lines if they do not exceed the maximum line length.

Finally, the other message placement, before yield, makes more sense, but I would change the wording to "Starting to read [...]" and put a full stop at the end of the message.

done, but I would put some brief logger info to indicate in the beginning of some stages (reading file, apply masking, separating epochs, etc.)

Sure, but not in "io.py".

Why not? I think within the io methods are the best places to go.
BTW, I am feeling that io.load_images_from_dir and io.load_images should be merged.

anyway, I will leave io.py intact

Because they are not useful. Why would a function log an info-level message saying it has been called every time it is called? Such messages could be emitted by the caller, if relevant.

We can discuss merging the load functions, but I would rather not, because "_dir" requires a suffix as well, so we would need to explain that.

well, the suffix can be an optional argument. Let's discuss this in the next standup.

mihaic · 2017-04-27T17:08:45Z

            np.sum(mask)
        )
-        images = io.load_images_from_dir(data_dir, extension)
+        images = io.load_images_from_dir(data_dir, suffix=extension)


It makes more sense to rename extension to suffix here.

mihaic · 2017-04-27T17:09:16Z

    mask_file = sys.argv[3]
    epoch_file = sys.argv[4]
-    images = io.load_images_from_dir(data_dir, extension)
+    images = io.load_images_from_dir(data_dir, suffix=extension)


mihaic · 2017-04-27T19:59:52Z

    for f in files:
+        logger.debug(
+            'starts to read file %s', f
+        )


Please write sentences in log messages, at least in this file.

…tation

xoltar · 2017-04-28T18:21:49Z

+            data_list[i] = _randomize_single_subject(data_list[i], seed=i)
+    elif random == RandomType.UNREPRODUCIBLE:
+        for i in range(len(data_list)):
+            data_list[i] = _randomize_single_subject(data_list[i])


This is better, but still mixed usage. If you're going to do the change in place, don't return a value. If you're going to return a value, it should be a new value. So if you want to use in-place, it should look like:

for subject in data_list: _randomize_single_subject(subject)

So neither _randomize_single_subject nor this function should return a value. Also, note since you're modifying in place, you don't need to assign to indexes, so you can use the simpler for loop.

xoltar · 2017-04-28T19:14:41Z

+    seed: Optional[int]
+        Seed for random state used implicitly for shuffling.
+
+    Returns


Good! Now just need to remove the Returns section from docs of both of these functions since they don't return anything anymore.

add random permutation to FCMA

81972f7

yidawang requested a review from mihaic April 27, 2017 05:30

xoltar requested changes Apr 27, 2017

View reviewed changes

addressing review comments from Bryn

507bbf3

mihaic requested changes Apr 27, 2017

View reviewed changes

yidawang added 2 commits April 27, 2017 11:21

addressing review comments from Mihai

5f04e95

further changes according to the comments

cec3445

mihaic requested changes Apr 27, 2017

View reviewed changes

addressing review comments

ec340ec

mihaic approved these changes Apr 27, 2017

View reviewed changes

yidawang added 2 commits April 27, 2017 16:33

Merge remote-tracking branch 'upstream/master' into fcma-random-permu…

0e28424

…tation

Merge remote-tracking branch 'upstream/master' into fcma-random-permu…

e90671a

…tation

xoltar reviewed Apr 28, 2017

View reviewed changes

addressing Bryn's comments

11383d1

xoltar reviewed Apr 28, 2017

View reviewed changes

yidawang added 2 commits April 28, 2017 12:20

updating docstrings

ec78ccd

fix building error issue

e8d60a8

xoltar approved these changes Apr 28, 2017

View reviewed changes

xoltar merged commit 3a4c566 into brainiak:master Apr 28, 2017

yidawang deleted the fcma-random-permutation branch April 29, 2017 03:37

Conversation

yidawang commented Apr 27, 2017

Uh oh!

xoltar 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!

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!

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!

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!

Reviewers

Uh oh!