Can one of the admins verify this patch?

Jenkins, add to whitelist.

@hugorichard, thank you very much for the PR.

Hopefully the SRM authors and contributors will be able to review it soon. @TuKo, @hejiaz, @cameronphchen?

The tests failed because there is the joblib package missing (which I use for multi-processing).
However it is in the requirement-dev.txt . It works when i run ./pr-check on my end. Is this normal ? What should I do to fix this ?
Hugo

If you are adding it as a dependency of BrainIAK (not just for testing), you must add it to install_requires in setup.py. You should then remove it from requirement-dev.txt, because we do not duplicate BrainIAK dependencies there.

Thanks ! I corrected this and it still does not run. It seems to be related to MPI although I do not make use of it:

from mpi4py import MPI
E   ImportError: dlopen(/Users/travis/miniconda3/conda-bld/brainiak_1556786193685/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho/lib/python3.6/site-packages/mpi4py/MPI.cpython-36m-darwin.so, 2): Library not loaded: @rpath/libmpi.1.dylib

I'm looking into the error. Please ignore it for the moment, since it only affects the Conda build.

Note that typing should not be a dependency, because it is included in the standard library starting with Python 3.5, which is the minimum required version for BrainIAK.

Sorry for the last remark. I saw typing in the diff and though you added it.

@qihongl, I hear you are also knowledgeable about SRM. Review maybe?

@hugorichard We were busy with the tutorials (https://brainiak.org/tutorials) release, but @qihongl will review this soon.

@manojneuro Thank you for your answer ! Don't hesitate to contact me if you need more information.

@hugorichard Thank you for your pull request. I'm working on SRM development, so I could try to review this pull request. Do you have any paper/write-up about this method? A paper about it would be really helpful when reviewing this request. Thanks!

I have just uploaded a paper on Arxiv: https://arxiv.org/pdf/1909.12537.pdf

(Although I have pushed some commits it still needs some work before a second pass can be done I'll tell you when I am done)

@hejiaz Now I have pushed a stable and fully tested version. I still need to update the example so that it complies with the new API. I did a bit more than requested so it was a lot of work but now some new features are available:

• temporary files can (and should !) be cleaned when the fitted model is not used anymore by calling the .clean method
• various input format are accepted output shapes are coherent with input shapes
• aggregate attribute (shared responses are averaged if aggregate is set to "mean" (default) otherwise a shared response per subject is returned like in DetSRM or ProbSRM)
• a method add_subjects is similar to transform_subject in DetSRM. the difference is that the additional subject is directly add to internal basis_list so that everything behaves like if this subject was present when fitting
• when no atlas is provided FastSRM defaults to DetSRM however low ram mode is still available so that this provides a way to apply vanillia DetSRM even if the data is difficult to hold in memory.

Update: I don't get why test don't pass online. On my computer when I run pytest test_fastsrm.py everything works fine and I get no segmentation fault.

@hugorichard, it might be related to running under MPI. Could you please try pr-check.sh as well on your computer?

You are right It seems related to MPI.
It does not run on my computer either. py.test works but if I do mpiexec -n 2 py.test it does not seem to work anymore.

I think I know it is because of joblib. It does not support nested parallelization loops. If I remove the concerned tests then it works. Should I do that ?

If joblib does not support running under MPI (a reference would be helpful), then we should test it separately, indeed. One small issue is making sure the coverage report is correct. I will look into it next week, unless you have experience with Coverage.py.

./pr-checks now works locally. It still does not work online though.

We are seeing problems with the Conda builds in PR #440 as well, so it is not related to the changes. I am investigating.

@hugorichard The new changes look good. I will accept it after Mihai fixed the Conda build problem.

This last commit just contains an updated example notebook that is consistent with the API (I also included wget as an example requirement to get a nice progress bar when people download the data used for the example but I can remove it if necessary).

@hugorichard, we fixed the build problem. Please update your PR branch from master.

@hejiaz, all tests are passing.

@hugorichard, thank you very much for your contribution! We look forward to continuing the collaboration.

@mihaic You are very welcome. Thanks for your help and thanks to the reviewers !