Update ISC permutation doc to clarify pairwise groups#476
Conversation
|
Oops accidentally created this branch off the |
|
Great, Tristan volunteered for this |
tristansyates
left a comment
There was a problem hiding this comment.
This looks great! I will say that it took me a second to fully understand the description of the input for permutation_isc. One small tweak that would help is to change the tense of the sentence : "In the pairwise approach, pairwise ISCs should be computed across both groups at once..." to the past tense (should have been). It may also be good to put the description of the shape of the input in either the beginning or the end of the paragraph instead of in both places
c1134fb to
f7514b4
Compare
|
@tristansyates good point! I fixed the tense. I think I'll leave the somewhat redundant input shape descriptions... the earlier one is intended to clarify that pairwise ISCs should have been computed in such a way as to produce a distance matrix containing both groups; the latter more specifically describes the input shapes (which for the pairwise approach should be distance vectors, not square). |
|
@snastase, the current changes look off. If you try to fix the code, start with a pull from GitHub because I used the update branch from master button. |
@mihaic—not sure I totally understand. Does something in particular look off to you? It looks correct to me... The only weirdness is that the changes from #475 are also part of the diff here. If we merge #475, this will only have a small diff—i.e. the only real diff in this PR is the two added lines 1070-1071 in the docstring. |
|
I thought the main change of this PR was introduced in commit I thought that change was missing in the current diff. However, I noticed that change was actually merged with PR #477. If you only need lines 1070-1071, we can merge them right away if you remove the changes from #475. Or we can wait for #475. |
This addresses the confusion with issue #449 (via @mcdoar9). The documentation was unclear on how to supply pairwise ISCs for two groups to the permutation test. The documentation is now updated to make it more clear that pairwise ISCs must be computed on both groups at once (such that between-group ISCs are included in the pairwise ISC matrix) prior to this ISC matrix being supplied to
permutation_isc.