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

Searchlight improvement #221

Merged
merged 15 commits into from May 10, 2017
Merged

Conversation

@mjanderson09
Copy link
Contributor

@mjanderson09 mjanderson09 commented Apr 28, 2017

No description provided.

@@ -33,7 +78,7 @@ class Searchlight:
Optionally, users can define a block function which runs over
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):

This comment has been minimized.

@mihaic

mihaic Apr 28, 2017
Contributor

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

This comment has been minimized.

@mihaic

mihaic Apr 28, 2017
Contributor

Does this mean the diamond contains exactly that number of voxels?

@@ -46,8 +91,13 @@ def __init__(self, sl_rad=1, max_blk_edge=10):
"""
self.sl_rad = sl_rad
self.max_blk_edge = max_blk_edge
if max_blk_edge == -1:

This comment has been minimized.

@mihaic

mihaic Apr 28, 2017
Contributor

Why not let the user specify max_blk_edge=sl_rad*3 as parameter?

@@ -0,0 +1,80 @@
# Copyright 2016 Intel Corporation

This comment has been minimized.

@mihaic

mihaic Apr 28, 2017
Contributor

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():

This comment has been minimized.

@mihaic

mihaic Apr 28, 2017
Contributor

These tests should be called test_searchlight_with_cube/diamond, because they test the shapes' usage in the searchlight, not the shape classes themselves.

This comment has been minimized.

@mihaic

mihaic Apr 28, 2017
Contributor

Not addressed.

This comment has been minimized.

@mjanderson09

mjanderson09 May 1, 2017
Author Contributor

OK I've made this change. Thank you.

searchlight cube, not counting the center voxel
"""
super(Cube, self).__init__(rad)

This comment has been minimized.

@mihaic

mihaic Apr 28, 2017
Contributor

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)

This comment has been minimized.

@mihaic

mihaic Apr 28, 2017
Contributor

Could you please document data as a class attribute in the superclass?

@mihaic
Copy link
Contributor

@mihaic mihaic commented May 1, 2017

@yidawang, it looks like the size of the output array has changed for 26 to 23 in "test_mvpa_voxel_selection". Any idea why?

Copy link
Contributor

@mihaic mihaic left a comment

If you could make the following changes, it would help with clarity.

@@ -345,18 +425,22 @@ def _singlenode_searchlight(l, msk, mysl_rad, bcast_var):
mysl_rad:-mysl_rad,
mysl_rad:-mysl_rad]

def check_mask(mask_cube):

This comment has been minimized.

@mihaic

mihaic May 1, 2017
Contributor

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

This comment has been minimized.

@mihaic

mihaic May 1, 2017
Contributor

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

This comment has been minimized.

@mihaic

mihaic May 1, 2017
Contributor

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)?

@@ -345,18 +425,22 @@ def _singlenode_searchlight(l, msk, mysl_rad, bcast_var):
mysl_rad:-mysl_rad,
mysl_rad:-mysl_rad]

def check_mask(mask_cube):
res = mask_cube[self.shape]
return np.any(res) and np.all(res)

This comment has been minimized.

@mihaic

mihaic May 1, 2017
Contributor

Isn't np.all sufficient?

This comment has been minimized.

@mjanderson09

mjanderson09 May 3, 2017
Author Contributor

No, because it returns true for an empty array.

This comment has been minimized.

@mihaic

mihaic May 3, 2017
Contributor

Then how about:

return len(res) > 0 and np.all(res)
mjanderson09 added 3 commits May 3, 2017
@mihaic
mihaic approved these changes May 10, 2017
@mihaic mihaic merged commit 34f59cd into brainiak:master May 10, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
linux Build finished.
Details
macos Build finished.
Details
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