Add 'side' option to randomization tests#477
Conversation
|
Great looks good Sam, do you have any recommendations for reviewers? |
|
This is a fairly straightforward change, so probably anybody could review. Not sure @me-sh is available; maybe if @tristansyates is actively running ISCs right now, they could eyeball this as well? |
tristansyates
left a comment
There was a problem hiding this comment.
This is a fairly straightforward change, so probably anybody could review. Not sure @me-sh is available; maybe if @tristansyates is actively running ISCs right now, they could eyeball this as well?
Great change! Everything looks good to me
|
Got it. We included #466 in the next release milestone. Do you have an estimate for agreeing on a way to close it? |
| Number of permutation iteration (randomizing group assignment) | ||
|
|
||
| side : str, default: 'two-sided' | ||
| Perform one-sided ('left' or 'right') or 'two-sided' test |
There was a problem hiding this comment.
The side default described in this docstring is not the default assigned in the function declaration.
More generally, we should not describe defaults in the docstrings because their presence in declarations makes them visible both in the Python built-in help function and the HTML docs:
python3 -c "import brainiak.isc; help(brainiak.isc.permutation_isc)"
https://brainiak.org/docs/brainiak.html#brainiak.isc.permutation_isc
|
|
||
| side : str, default: 'two-sided' | ||
| Perform one-sided ('left' or 'right') or 'two-sided' test | ||
|
|
| n_shifts : int, default: 1000 | ||
| Number of randomly shifted samples | ||
|
|
||
| side : str, default: 'two-sided' |
|
Thanks @mihaic—good catch! I didn't realize this about defaults in the docstring... so should I remove the |
|
I think we should have dedicated PRs for such cleanups. For now, lets just not add more default descriptions, so just remove the ones part of the diff. |
|
Okay, I removed the defaults from the docstring for each occurrence of this |
This adds a
side(possible values:two-tailed,right,left) option to be passed top_from_nullin the ISC randomization tests for determining whether the p-value output is one- or two-tailed (based on conversations with @me-sh). Previously,two-tailedwas hardcoded into the randomization tests. I've also set the default toright, since effectively all ISC analyses are seeking positive correlation (and negative correlations are pretty nonsensical in ISC analysis).