Can one of the admins verify this patch?

Jenkins, add to whitelist.

@snastase, please ignore the Travis status for the moment. The failure is unrelated. I'm working on a fix.

Hi, folks @snastase @mihaic sorry about the delayed response! And thank you very much @snastase
! I'm very excited about this pull request! Though my schedule is pretty crazy recently. If this is not time sensitive, I'd love to help with anything!

@mihaic @snastase I probably should discuss this point somewhere else:

This pull request raised a great point - standardized data geometry. I guess we probably should consider standardizing the input data geometry to be consistent with sklearn. For example, SRM currently takes n_features (n_voxels) x n_exampels (n_TRs), whereas sklearn, as well as many standard ML textbooks, use the transposed input format n_exampels x n_features. This have been causing some confusion in the past.

Moreover, I think we should encourage all future packages to be consistent with some default data format.

@snastase thanks for working on this. Meir and David Turner are doing some tests on memory utilization of ISC with Searchlight. I'll request them to use this fix as a part of their tests. This will provide an extra validation on many of these updates.

@qihongl, I agree about the geometry comment. See issue #356 for a different idea.

@qihongl @mihaic I definitely agree about having a standard input geometry, and any transposes, slicing, etc happening internally. I would advocate for n_samples (typically time points, conditions) by n_features (typically voxels, electrodes, surface vertices), as this seems to be the most widespread convention and makes the most sense for, e.g., brain decoding analyses. I think standardizing this throughout will be critical for overall usability of BrainIAK software. I'm not very familiar with xarrays, but they look very cool. Seems like the ultimate solution would be to generally assume a standardized 2D geometry for neural data (with checks) but also allow for the increased flexibility / explicitness of xarrays.

@manojneuro I haven't put a lot of effort into optimizing for speed / computing resources yet—was going for flexibility and readability—so I suspect there are several points where we could speed things up! Current version may run slower than the previous implementation until then, but I haven't thoroughly tested this.

@snastase, regarding your f-string commit, I created issue #393 about the minimum version of dependencies we should support.

I think removing (not deprecating) the old module brainiak.isfc is actually the way to go considering we are not keeping any function signature unchanged.

In any case, the current approach of duplicating the code is not what we want.

Okay, will remove the brainiak.isfc duplicate (and tests) entirely.

I'm a bit confused about how to handle the current test error regarding linked references in the docstring. I'm getting the following message:

Warning, treated as error:
/home/travis/build/brainiak/brainiak/brainiak/isc.py:docstring of brainiak.isc.permutation_isc:30:Duplicate explicit target name: "chen2016".
make: *** [html] Error 2
/home/travis/build/brainiak/brainiak
make docs failed
The command "./pr-check.sh" exited with 1.

Is this because I have multiple pointers to the same reference? I.e., the Chen2016 reference is used for both bootstrap_isc and permutation_isc. What's the solution to this?

@mihaic also pointed out in an offline conversation that we'll need to adjust the ISC example (https://github.com/brainiak/brainiak/blob/master/examples/isfc/isfc.py) to match the new functionality.

The error is because of multiple definitions of chen2016. You should remove the second of the definitions and replace it with a reference, e.g.:
The implementation is based on the following publication: [Chen2016]_.
http://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#citations

Hey @mihaic, some tests are failing for linux during the conda build, apparently due to problems with pymanopt(?)—any idea what that's about?

It's not related to your PR (I can see the same error on my machine on the master branch). I'm looking into it, but if you're done before I figure it out, we can merge your PR anyway.

@mihaic I think all the base functionality @qihongl reviewed is basically ready to go. There are other related topics (e.g., updating the examples)... not sure if those are best handled here or in another PR.

The master branch is supposed to be functional, so I don't think we can postpone updating the examples.

Also, why do you think this test failed on Linux on Jenkins?

=================================== FAILURES ===================================
_______________________________ test_isc_output ________________________________

    def test_isc_output():
    
        logger.info("Testing ISC outputs")
    
        data = correlated_timeseries(20, 60, noise=0,
                                     random_state=42)
        iscs = isc(data, pairwise=False)
        assert np.all(iscs[:, :2] == 1.)
        assert np.all(iscs[:, -1] < 1.)
    
        iscs = isc(data, pairwise=True)
>       assert np.all(iscs[:, :2] == 1.)
E       assert False
E        +  where False = <function all at 0x7f6aef557e18>(array([[1., 1.],\n       [1., 1.],\n       [1., 1.],\n       [1., 1.],\n       [1., 1.],\n       [1., 1.],\n       [1., 1.],...       [1., 1.],\n       [1., 1.],\n       [1., 1.],\n       [1., 1.],\n       [1., 1.],\n       [1., 1.],\n       [1., 1.]]) == 1.0)
E        +    where <function all at 0x7f6aef557e18> = np.all

@mihaic Okay, the ISC/ISFC example is now functional and updated to use the new code (including new MaskMultiSubjectData shape, etc). I'm not sure why that test would fail. The point was to ensure that the correlation between identical time series was exactly 1. I haven't been able to recreate a situation where the test failed locally. I guess it's possible that the np.corrcoef computation could introduce some tiny deviation and thus break the assertion? I switched the assertion from testing for strictly 1 to np.allclose to introduce a tiny bit of tolerance.