Skip to content
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

Fcma mvpa searchlight #154

Merged
merged 10 commits into from Jan 12, 2017
Merged

Fcma mvpa searchlight #154

merged 10 commits into from Jan 12, 2017

Conversation

@yidawang
Copy link
Member

@yidawang yidawang commented Jan 11, 2017

enable activity-based voxel selection in the FCMA pipeline

the condition labels of the epochs
len(labels) labels equals the number of epochs
assuming the epochs of the same sid are adjacent,
and sid is the index of raw\_data

This comment has been minimized.

@mihaic

mihaic Jan 11, 2017
Contributor

Could you please explain more the meaning of sid?

This comment has been minimized.

@yidawang

yidawang Jan 11, 2017
Author Member

done

mask: 3D array
epoch\_info: list of tuple (label, sid, start, end)
the condition labels of the epochs

This comment has been minimized.

@mihaic

mihaic Jan 11, 2017
Contributor

Please format this text like a normal paragraph, with sentences. Of course, you can use a bulletted list too if you think it helps.

This comment has been minimized.

@yidawang

yidawang Jan 11, 2017
Author Member

fixed

processed\_data: 4D array in shape [brain 3D + epoch]
contains the averaged and normalized brain data epoch by epoch
it is generated in _\preprocess\_data method

This comment has been minimized.

@mihaic

mihaic Jan 11, 2017
Contributor

You must not reference private APIs (_preprocess_data) in the documentation of public APIs (MVPAVoxelSelector).

This comment has been minimized.

@yidawang

yidawang Jan 11, 2017
Author Member

fixed

sl: Searchlight
the distributed Searchlight object
processed\_data: 4D array in shape [brain 3D + epoch]

This comment has been minimized.

@mihaic

mihaic Jan 11, 2017
Contributor

If processed_data is not passed to __init__, it must not be documented as a parameter. How about listing it as as an attribute and adding naming it processed_data_, like in SRM?
https://github.com/IntelPNI/brainiak/blob/master/brainiak/funcalign/srm.py#L116
This would be in accordance with our coding standards:
http://scikit-learn.org/stable/developers/contributing.html#estimated-attributes

Same for labels.

On the other hand, the whole FCMA architecture does not fit our coding standards (data is passed at instantiation, instead of through fit), so maybe it is best to leave this code as it is for the moment and restructure all of FCMA later. Up to you. But note that the current list of parameters is confusing, because not all parameters listed are accepted by __init__.

This comment has been minimized.

@yidawang

yidawang Jan 11, 2017
Author Member

The parameter list here documents the properties of MVPAVoxelSelector instead of the input of __init__. The two voxel selector of FCMA is not about fitting, so I didn't follow the standard to only pass data through fit. I agree to keep the code structure as it is and leave the restructure discussion later. For now, let me modify the properties not passed through __init__ as var_.

self.mask = mask.astype(np.bool)
self.epoch_info = epoch_info
self.num_folds = num_folds
self.sl = Searchlight(sl_rad=sl_rad, max_blk_edge=max_blk_edge)

This comment has been minimized.

@mihaic

mihaic Jan 11, 2017
Contributor

Why not let the user created the searchlight? Then you would not need to accept (and document) all the searchlight parameters in your __init__?

This comment has been minimized.

@yidawang

yidawang Jan 11, 2017
Author Member

done

epoch\_info: list of tuple (label, sid, start, end)
the condition labels of the epochs
len(labels) labels equals the number of epochs
assuming the epochs of the same sid are adjacent

This comment has been minimized.

@mihaic

mihaic Jan 11, 2017
Contributor

Please write sentences in the docstrings. They are hard to follow without capitalization and punctuation.

This comment has been minimized.

@yidawang

yidawang Jan 11, 2017
Author Member

done

logger.info(
'epoch separation done, takes %.2f s' %
(time2 - time1)
)

This comment has been minimized.

@mihaic

mihaic Jan 11, 2017
Contributor

Isn't debug more appropriate for timing messages?

This comment has been minimized.

@yidawang

yidawang Jan 11, 2017
Author Member

done

example running command:
mpirun -np 2 python mvpa_voxel_selection.py /Users/yidawang/data/face_scene/raw nii.gz /Users/yidawang/data/face_scene/mask.nii.gz
data/fs_epoch_labels.npy 18
"""

This comment has been minimized.

@mihaic

mihaic Jan 11, 2017
Contributor

How about providing some sample data like in SRM?
https://github.com/IntelPNI/brainiak/blob/master/examples/funcalign/download-data.sh

If the files are small enough (a few kB), you can even add them to Git.

This comment has been minimized.

@mihaic

mihaic Jan 11, 2017
Contributor

Also, how about making a shell script with the command, like for HTFA?
https://github.com/IntelPNI/brainiak/blob/master/examples/factoranalysis/run_mpi_htfa.sh

This comment has been minimized.

@yidawang

yidawang Jan 11, 2017
Author Member

good idea, done

@mihaic
mihaic approved these changes Jan 12, 2017
@mihaic mihaic merged commit 1bd49d4 into brainiak:master Jan 12, 2017
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux Build finished.
Details
macos Build finished.
Details
@yidawang yidawang deleted the yidawang:fcma_mvpa_searchlight branch Jan 12, 2017
danielsuo pushed a commit that referenced this pull request Nov 16, 2017
danielsuo pushed a commit that referenced this pull request Nov 16, 2017
* Improve formatting of error messages.

* Catch errors that occur when looking up function name from function ID.

* Push warning to user if worker spends to long waiting for proper import counter.

* Fixes.

* Add comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants