Repository navigation
Searchlight improvement - #221
Conversation
| larger portions of the volume called blocks. | ||
| """ | ||
| def __init__(self, sl_rad=1, max_blk_edge=10): | ||
| def __init__(self, sl_rad=1, max_blk_edge=-1, shape=Cube): |
There was a problem hiding this comment.
Please document the shape parameter. I suggest you add an empty superclass from which both Cube and Diamond inherit, which you can use to document the type of the parameter.
| class Diamond: | ||
| """Diamond | ||
|
|
||
| Searchlight shape which is a (2*rad+1)^3 diamond |
There was a problem hiding this comment.
Does this mean the diamond contains exactly that number of voxels?
| """ | ||
| self.sl_rad = sl_rad | ||
| self.max_blk_edge = max_blk_edge | ||
| if max_blk_edge == -1: |
There was a problem hiding this comment.
Why not let the user specify max_blk_edge=sl_rad*3 as parameter?
| @@ -0,0 +1,80 @@ | |||
| # Copyright 2016 Intel Corporation | |||
There was a problem hiding this comment.
I think theses tests should be part of "test_searchlight.py".
| from brainiak.searchlight.searchlight import Searchlight | ||
| from brainiak.searchlight.searchlight import Diamond | ||
|
|
||
| def test_cube(): |
There was a problem hiding this comment.
These tests should be called test_searchlight_with_cube/diamond, because they test the shapes' usage in the searchlight, not the shape classes themselves.
There was a problem hiding this comment.
OK I've made this change. Thank you.
| searchlight cube, not counting the center voxel | ||
|
|
||
| """ | ||
| super(Cube, self).__init__(rad) |
There was a problem hiding this comment.
You should call super() with no arguments (also in Diamond):
https://docs.python.org/3/library/functions.html#super
| """ | ||
| super(Cube, self).__init__(rad) | ||
| self.rad = rad | ||
| self.data = np.ones((2*rad+1, 2*rad+1, 2*rad+1), dtype=np.bool) |
There was a problem hiding this comment.
Could you please document data as a class attribute in the superclass?
|
@yidawang, it looks like the size of the output array has changed for 26 to 23 in "test_mvpa_voxel_selection". Any idea why? |
mihaic
left a comment
There was a problem hiding this comment.
If you could make the following changes, it would help with clarity.
| mysl_rad:-mysl_rad, | ||
| mysl_rad:-mysl_rad] | ||
|
|
||
| def check_mask(mask_cube): |
There was a problem hiding this comment.
Please make this a method and document it.
| ---------- | ||
|
|
||
| data_ : a 3D boolean numpy array of size (2*rad+1)^3 which is set | ||
| to True within the boundaries of the desired shape |
There was a problem hiding this comment.
As discussed, this is a mask, so it should be named appropriately, e.g., mask_.
| ---------- | ||
|
|
||
| data_ : a 3D boolean numpy array of size (2*rad+1)^3 which is set | ||
| to True within the boundaries of the desired shape |
There was a problem hiding this comment.
Could you please change the array description to use shape, like elsewhere in BrainIAK, e.g., of shape (2*rad+1, 2*rad+1, 2*rad+1)?
|
|
||
| def check_mask(mask_cube): | ||
| res = mask_cube[self.shape] | ||
| return np.any(res) and np.all(res) |
There was a problem hiding this comment.
No, because it returns true for an empty array.
There was a problem hiding this comment.
Then how about:
return len(res) > 0 and np.all(res)
No description provided.