Pairwise bootstrap ISC doesn't work #472

Activity

1. CameronTEllis commented on Jul 27, 2020

   

   CameronTEllis

   on Jul 27, 2020

   Contributor

   More actions

   Hello! From my understanding, you diagnosed the problem correctly: assert np.allclose(voxel_iscs, voxel_iscs.T) won't work if there are nans, so using np.nan_to_num 'fixed' that issue; however, it ran overnight and didn't finish because it is slow to run 1000 bootstraps. You could test if that is the problem by setting n_bootstraps to a small number (e.g. 5) and seeing if that finishes.

   More generally, I would be cautions about using np.nan_to_num. If you have nans, how do you want to deal with those? Are all voxels for a given TR nans or are some voxels all nans or is it something between?
2. snastase commented on Jul 27, 2020

   

   snastase

   on Jul 27, 2020

   Last edited by snastase

   Contributor

   More actions

   Hi @yibeichan, thanks for pointing this out! Like @CameronTEllis said, you're correct that it breaks when receiving nans specifically when pairwise=True because of the assert np.allclose(voxel_iscs, voxel_iscs.T) line. This is a bug. The original thought behind that assertion was to ensure that the user correctly inputs pairwise ISCs in distance vector format so that squareform properly yields a square symmetric matrix, but I didn't foresee the issue with nans.

   Thinking about how best to deal with this... Looking closer at the code, I think that the preceding _check_isc_input line already takes care of checking that pairwise ISC inputs are correctly formatted as a distance vector, which makes the assertion(s) in question superfluous. So I think they can be dropped completely. We can add force='tomatrix' to the squareform call, which I think will ensure the expected behavior, if we want to be extra safe. @CameronTEllis does this make sense to you?
3. CameronTEllis commented on Jul 28, 2020

   

   CameronTEllis

   on Jul 28, 2020

   Contributor

   More actions

   @snastase I agree with your suggested fix, it seems like that assertion is unnecessary given the other checks.
4. yibeichan commented on Jul 30, 2020

   

   yibeichan

   on Jul 30, 2020

   Author

   More actions

   Thank you all! @CameronTEllis yes, I have too many voxels. I tried it on a small set. It worked. What do you mean Are all voxels for a given TR nans or are some voxels all nans or is it something between? Since my input is an ISC matrix with the shape as (num of pairs, num of voxels), I don't have any TR information anymore. So what's your concern?

   @snastase do we really need to add force='tomatrix' to the squareform call in _check_isc_input ? Doesn't assert voxel_iscs.shape[0] == voxel_iscs.shape[1] in bootstrap_isc do the same thing?
5. yibeichan commented on Jul 30, 2020

   

   yibeichan

   on Jul 30, 2020

   Author

   More actions

   oh yeah, assert voxel_iscs.shape[0] == voxel_iscs.shape[1] only checks the shape, but force='tomatrix' actually returns you a matrix. But I thought a matrix is default if the input is in the correct shape?

   I'm just curious when we will get a vector, not a matrix from squareform() if we don't specify force='tovector'. I mean, do we have to add force='tomatrix' in _check_isc_input?
6. snastase commented on Jul 31, 2020

   

   snastase

   on Jul 31, 2020

   Contributor

   More actions

   @yibeichan squareform is kind of like a toggle: if you input a distance vector, it outputs the distance matrix; if you input a distance matrix, it outputs the distance vector. Those assert statements probably snuck in there when I was initially writing the code (and hadn't thought about force='tomatrix'). I don't think the force='tomatrix' is strictly necessary given the tests in _check_isc_input, but I don't mind a little redundancy if it makes the squareform easier to interpret.

   👍React with 👍1Reacted by Yibei Chen