Fcma by yidawang · Pull Request #97 · brainiak/brainiak

yidawang · 2016-08-28T04:40:11Z

add in correlation based voxel selection of FCMA, being able to reproduce the results original FCMA toolbox gets.
provide an example of using correlation based voxel selection of FCMA.
modify setup.py to enable CMake compiling.

…ch cube list

mihaic · 2016-08-29T16:42:52Z

+    raw_data = []
+    labels = []
+    for sid in range(len(epoch_list)):
+        epoch = epoch_list[sid] # epoch is a numpy array in shape (condition, nEpochs, nTRs)


The description of an epoch is already given in the function docstring.

Will remove it

TuKo · 2016-08-29T20:04:17Z

@yidawang: Did you find any performance difference with the njobs in sklearn?

yidawang · 2016-08-29T20:07:52Z

@TuKo Not much difference. What is worse, n_jobs=-1 will spawn a bunch of processes which uses up the memory especially when the dataset is large. In any case, we will come up with our own high performance SVM implementation, so I don't worry much about this. Thanks for the suggestion, though.

… Cython wrapper

…on, format the code

mihaic · 2016-08-31T22:38:59Z

@@ -0,0 +1,2 @@
+mpi4py
+sklearn


Only list packages that are not already listed in setup.py. In this case, there is no need for this requirements file.

…collecting the right coverage

mihaic · 2016-09-17T00:12:37Z

 .idea
 .ipynb_checkpoints
 junit-*.xml
+brainiak/fcma/__pycache__


How about __pycache__?

mihaic · 2016-09-17T00:15:05Z

+        if 'icc' in os.environ['CC']:
+            opts += ['-lirc', '-lintlc']
+    elif 'icc' in sysconfig.get_config_var('CC'):
+        opts += ['-lirc', '-lintlc']


Please combine the checks such that you don't repeat the assignment.

mihaic · 2016-09-17T00:22:04Z

 import os
 import sys
 import setuptools
+import subprocess


mihaic · 2016-09-17T00:24:03Z

+    if sys.platform == 'Windows':
+        raise RuntimeError("BrainIAK cannot be built on Windows")
+
+    # configuration for regular compiling


No need for these changes, we make it clear in the README we do not run on Windows. If you want, please submit a different PR with a comprehensive check for Windows, but I do not think it is necessary.

mihaic · 2016-09-17T00:28:59Z

+            if isinstance(ext, Extension):
+                self.regular_compiling(ext)
+
+    def regular_compiling(self, ext):


Is this necessary?

mihaic · 2016-09-17T01:03:26Z

+# make the cython building execute only once
+if MPI.COMM_WORLD.Get_rank() == 0:
+    from . import cython_blas as blas
+    blas.installed()


Is this necessary? Is the module not compiled at import?

This is about the MPI multi-process competition on the compiled library file we discussed before.

I meant the import on the previous line. Is it not compiled then?

not until the first use

mihaic · 2016-09-17T01:05:32Z

+    a = 'installed'
+# tell everybody the building is done
+# functions as a barrier
+a = MPI.COMM_WORLD.bcast(a, root=0)


Is it not sufficient to have a barrier here? Then you could remove the variable.

mihaic · 2016-09-17T04:52:54Z


 class get_pybind_include(object):
    """Helper class to determine the pybind11 include path
-


This blank line is supposed to be here (docstring = one-line summary + blank line + details).

mihaic · 2016-09-17T04:53:07Z


 def cpp_flag(compiler):
    """Return the -std=c++[11/14] compiler flag.
-


mihaic · 2016-09-19T18:18:53Z

+    opts = []
+    if ('CC' in os.environ and 'icc' in os.environ['CC']) or \
+        'icc' in sysconfig.get_config_var('CC'):
+            opts += ['-lirc', '-lintlc']


I looked for documentation on these libraries, but cannot find anything. Why do we need to add them explicitly?

They are to fix an Intel compiler related library link issue
sneumann/mzR#28 (comment)

Add more string-like methods required by Cython to pybind11 workaround class. Remove unused `pytest-cython`. Remove `-fvisibility=hidden` which conflicts with Cython.

Compile Cython code during install, not at runtime

mihaic · 2016-09-20T23:50:26Z

    packages=find_packages(),
+    package_data = {
+        'brainiak.fcma': ['*.pyx', '*.pyxbld'],
+    },


The pyx files corresponding to extensions are included by default. We do not use pyxbld files anymore. Please remove package_data.

mihaic · 2016-09-20T23:53:01Z

+    results1 = vs.run(clf)
+    # test scipy normalization
+    fake_corr = np.random.rand(1, 12, 100).astype(np.float32)
+    fake_corr = vs._correlationNormalization(fake_corr)


Please verify the result using an expected value.

mihaic · 2016-09-20T23:57:11Z

+    blas.ssyrk(uplo, trans, &N, &K, &alpha, &A[py_start_voxel, 0, 0], &lda,
+               &beta, &C[0, 0], &ldc)
+    # shrink the values for getting more stable alpha values in SVM training iteration
+    py_c *= .001


Shouldn't this be a parameter?

This is almost a fixed value. It gives us larger alpha values in SVM given our data shape (#features>>#samples). There is not much the users can tune about this.

mihaic · 2016-09-21T00:01:05Z

+    results0 = vs.run(clf)
+    # set master rank to be 1 and do it again
+    vs = VoxelSelector(fake_raw_data, 2, labels, 2, master_rank=1)
+    results1 = vs.run(clf)


Please verify the result using an expected value.

…MA tests

mihaic · 2016-09-21T01:13:15Z

        for ext in self.extensions:
            ext.extra_compile_args = opts
            ext.extra_link_args = opts
+            if ext.language is 'c++':


I suspect the tests are failing because of this comparison. It should be ==.

you are probably right, checking on it now

mihaic · 2016-09-21T01:44:31Z

        for ext in self.extensions:
            ext.extra_compile_args = opts
            ext.extra_link_args = opts
+            if ext.language == 'c++':


How about this to not have to define the extension language?

lang = ext.language or self.compiler.detect_language(ext.sources) if lang == 'c++':

…n data files

…move file reading in fcma test

mihaic · 2016-09-22T04:20:22Z

Jenkins, retest this please.

… the main thread. (#97)

yidawang added 15 commits August 16, 2016 19:08

add CMake support to the isntallation

08764d3

initialize fcma

17ab76a

first path of voxel selection with hard-coded path

1bfccc7

sort files before processing; only process the results in master

2e7c064

use self-defined mask applying method, instead of NiftiMasker

570bd15

add Fisher tranformation to the within subject normalization

0c19f91

fix a bug in Fisher Transformation

7b69c76

use C++ to do within-subject normalization, add running example

70c653d

check compiler type in CMake

1bb5282

enable finding blas library/MKL in different OSes

7cbad63

use new data structure to depict epoch infomation, add an example epo…

b9f4d9c

…ch cube list

simplify the fcma example, encapsulate MPI communicatation

54da6f0

broadcast the data subject by subject to prevent size overflow

d9ea112

minor bug fix in fcma example

38db4e3

formalize comments

3935570

mihaic reviewed Aug 29, 2016
View reviewed changes

yidawang added 2 commits August 29, 2016 10:45

impose scikit-learn svm cross validation to use all CPUs

aab70df

tiny bug fixed in the worker output

67d97d1

yidawang added 6 commits August 29, 2016 19:46

change to column major in preparation for using BLAS APIs under SciPy…

d0af529

… Cython wrapper

change the input data to row-major

55565af

minor bugs fixed

29f6d47

use blas wrapper by scipy cython

044d34c

format output

849a061

add in classifier flexibility, use regular compiling for fcma_extensi…

2bdd569

…on, format the code

mihaic reviewed Aug 31, 2016
View reviewed changes

yidawang added 2 commits August 31, 2016 18:25

add fcma test, modify run-tests.sh for testing out MPI processes and …

5d7ea8b

…collecting the right coverage

make two processes of fcma test exit in different time

193ce84

mihaic requested changes Sep 17, 2016

View reviewed changes

yidawang added 2 commits September 16, 2016 19:14

restore original setup.py and address other review comments

a77b51f

add fcma extension back

8f510f7

mihaic reviewed Sep 17, 2016

View reviewed changes

Comment thread setup.py

def cpp_flag(compiler):

"""Return the -std=c++[11/14] compiler flag.

mihaic Sep 17, 2016

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

fortmatting issue

44225eb

mihaic reviewed Sep 19, 2016

View reviewed changes

yidawang and others added 3 commits September 19, 2016 13:58

copy *pyxbld files when installing

f65f6c8

Compile Cython code during install, not at runtime

ef554a2

Add more string-like methods required by Cython to pybind11 workaround class. Remove unused `pytest-cython`. Remove `-fvisibility=hidden` which conflicts with Cython.

Merge pull request #5 from mihaic/fcma-compile-at-install

0343ade

Compile Cython code during install, not at runtime

mihaic requested changes Sep 21, 2016

View reviewed changes

yidawang added 2 commits September 20, 2016 18:04

only add c++11/14 to C++ extension compiling; check correctness in FC…

0b7b078

…MA tests

explicitly specify language for each extension

5eadef1

mihaic requested changes Sep 21, 2016

View reviewed changes

yidawang added 2 commits September 20, 2016 18:23

use == to check if two strings equal

39f9f07

set icc flags to c_opts correctly

031d5fb

mihaic reviewed Sep 21, 2016

View reviewed changes

yidawang added 9 commits September 20, 2016 19:00

detect extension language in a smarter way; let fcma test code read i…

3d86e86

…n data files

use deep copy to assign compiling flags

f862c9b

switch the test sequence

f75d9f7

make kernel matrix scale setting adjustable to the initial values; re…

257882d

…move file reading in fcma test

tolerate one sample error in cross validation

3f14b80

delete unnecessary data file; improve fcma test coverage

9a3f00b

add logistic regression to fcma test

03c6957

rename the code file in fcma example

e4823b1

Merge remote-tracking branch 'upstream/master' into fcma

d0204ab

mihaic merged commit e50068d into brainiak:master Sep 22, 2016

yidawang deleted the fcma branch September 22, 2016 16:58

danielsuo pushed a commit that referenced this pull request Nov 16, 2017

Throw an exception if a Ray method is called from a thread that isn't…

0f70910

… the main thread. (#97)


		class get_pybind_include(object):
		"""Helper class to determine the pybind11 include path


		def cpp_flag(compiler):
		"""Return the -std=c++[11/14] compiler flag.

		@@ -0,0 +1,2 @@
		mpi4py
		sklearn

Conversation

yidawang commented Aug 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TuKo commented Aug 29, 2016

Uh oh!

yidawang commented Aug 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaic Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaic commented Sep 22, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mihaic Sep 20, 2016 •

edited

Loading