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] sys.exit used in _get_block_data #156

Closed
mihaic opened this Issue Jan 11, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@mihaic
Copy link
Contributor

mihaic commented Jan 11, 2017

https://github.com/IntelPNI/brainiak/blob/master/brainiak/searchlight/searchlight.py#L136
Instead, an exception should be raised (and documented) in distribute if the input data does not have the proper shape.

@YSanchezAraujo

This comment has been minimized.

Copy link
Contributor

YSanchezAraujo commented Jun 21, 2017

@mihaic are you saying the exception should be raised here instead of here

@mihaic

This comment has been minimized.

Copy link
Contributor Author

mihaic commented Jun 21, 2017

@YSanchezAraujo, thanks for looking into this issue.

Indeed, I am saying that an exception should be raised in distribute, such that the user code can decide what to do, instead of exiting the program as we are doing now. This is the approach we are using everywhere else in BrainIAK.

In practice, in addition to raising an exception in _get_block_data, the exception should be caught and a new one raised in distribute (so the user does not see the implementation details in the traceback).

Instead, it would be simpler to check the input shape in distribute and just remove the else code from _get_block_data.

@YSanchezAraujo

This comment has been minimized.

Copy link
Contributor

YSanchezAraujo commented Jun 21, 2017

thanks for the response, some questions:

is the mask argument guaranteed to be a numpy array, and or the 4d arrays inside subjects, should there be an instance check and maybe a np.array(<argument>) if isinstance(<argument>, np.ndarray) returns false?

thinking of something like:

assert mask.ndim == 3, "mask should be a 3D array"

for (idx, subj) in enumerate(subjects):
    assert subj.ndim == 4, \
    "expected 4D data for index {} in subjects".format(idx)
@mihaic

This comment has been minimized.

Copy link
Contributor Author

mihaic commented Jun 21, 2017

We should also check mask. And arguments should be np.ndarray instances. It is better if users explicitly apply np.array to other data structures themselves and deal with potential issues.

@mihaic mihaic closed this in e2dd0a7 Jun 23, 2017

danielsuo pushed a commit that referenced this issue Nov 16, 2017

Use redismodules for task table and result table. (#156)
* Switch to using redis modules for task table.

* Switch to using redis modules for the task table.

* Fix some tests.

* Fix naming and remove code duplication.

* Remove duplication in redis modules and add more cleanups.

* Address comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment