Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fcma #97

Merged
merged 63 commits into from Sep 22, 2016
Merged

Fcma #97

merged 63 commits into from Sep 22, 2016

Conversation

@yidawang
Copy link
Member

@yidawang yidawang commented Aug 28, 2016

  1. add in correlation based voxel selection of FCMA, being able to reproduce the results original FCMA toolbox gets.
  2. provide an example of using correlation based voxel selection of FCMA.
  3. modify setup.py to enable CMake compiling.
raw_data = []
labels = []
for sid in range(len(epoch_list)):
epoch = epoch_list[sid] # epoch is a numpy array in shape (condition, nEpochs, nTRs)

This comment has been minimized.

@mihaic

mihaic Aug 29, 2016
Contributor

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

This comment has been minimized.

@yidawang

yidawang Aug 29, 2016
Author Member

Will remove it

else:
raise RuntimeError("Unsupported operating system")
try:
self.blas_library = ctypes.cdll.LoadLibrary('libblas'+extension)

This comment has been minimized.

This comment has been minimized.

@mihaic

mihaic Aug 29, 2016
Contributor

It looks like gemm parameter ldc is always fixed:
https://github.com/scipy/scipy/blob/master/scipy/linalg/fblas_l3.pyf.src#L24

This comment has been minimized.

@TuKo

TuKo Aug 29, 2016
Contributor

I think that the best option to avoid platform issues here is to have a c wrapper calling the blas library as needed. In that way you can also call the C interface of blas. What do you think?

This comment has been minimized.

@mihaic

mihaic Aug 29, 2016
Contributor

Luckily for us, SciPy added such an interface last year:
http://docs.scipy.org/doc/scipy/reference/linalg.cython_blas.html
scipy/scipy#4021

This comment has been minimized.

@yidawang

yidawang Aug 29, 2016
Author Member

I still feel that we should find a more elegant way to load the C++ compiled BLAS dynamic libraries. SciPy eventually uses MKL or OpenBlas, etc., we should be able to do the same thing.

This comment has been minimized.

@yidawang

yidawang Aug 29, 2016
Author Member

@TuKo Yes, this is what I meant. I will write some C++ wrapper which finds the library when installing.

This comment has been minimized.

@mihaic

mihaic Aug 29, 2016
Contributor

@yidawang What does your wrapper provide that is missing from the wrapper provided by SciPy?

This comment has been minimized.

@yidawang

yidawang Aug 29, 2016
Author Member

@mihaic using C++ row major matrix

This comment has been minimized.

@mihaic

mihaic Aug 29, 2016
Contributor

The underlying BLAS code is the same. It does not matter if you access it through the Fortran or C interface, the expected memory arrangement is the same. Depending on the implementation, using row-major or column-major matrices may be faster. However, the interface used to access the BLAS implementation does not matter.

You may want to use a trick such as this one suggested by Jeff if the performance difference is large (again, this does not depend on the interface):
http://scicomp.stackexchange.com/a/14407

So I do not see this as a reason to ignore the existing SciPy Cython wrapper and instead write and maintain our own.

This comment has been minimized.

@yidawang

yidawang Aug 29, 2016
Author Member

@mihaic Well, those points are well-taken. Let me use SciPy's wrapper then, although it will take additional effort from my side.

@TuKo
Copy link
Contributor

@TuKo TuKo commented Aug 29, 2016

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

@yidawang
Copy link
Member Author

@yidawang yidawang commented Aug 29, 2016

@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.

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

This comment has been minimized.

@mihaic

mihaic Aug 31, 2016
Contributor

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

@@ -20,7 +20,6 @@

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

This comment has been minimized.

@mihaic

mihaic Sep 17, 2016
Contributor

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

@@ -64,7 +72,6 @@ def has_flag(compiler, flagname):

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

This comment has been minimized.

@mihaic

mihaic Sep 17, 2016
Contributor

Same.

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

This comment has been minimized.

@mihaic

mihaic Sep 19, 2016
Contributor

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

This comment has been minimized.

@yidawang

yidawang Sep 19, 2016
Author Member

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

yidawang and others added 3 commits Sep 19, 2016
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
@@ -121,5 +152,8 @@ def build_extensions(self):
ext_modules=ext_modules,
cmdclass={'build_ext': BuildExt},
packages=find_packages(),
package_data = {
'brainiak.fcma': ['*.pyx', '*.pyxbld'],
},

This comment has been minimized.

@mihaic

mihaic Sep 21, 2016
Contributor

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

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

This comment has been minimized.

@mihaic

mihaic Sep 21, 2016
Contributor

Please verify the result using an expected value.

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

This comment has been minimized.

@mihaic

mihaic Sep 21, 2016
Contributor

Shouldn't this be a parameter?

This comment has been minimized.

@yidawang

yidawang Sep 21, 2016
Author Member

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.

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)

This comment has been minimized.

@mihaic

mihaic Sep 21, 2016
Contributor

Please verify the result using an expected value.

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

This comment has been minimized.

@mihaic

mihaic Sep 21, 2016
Contributor

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

This comment has been minimized.

@yidawang

yidawang Sep 21, 2016
Author Member

you are probably right, checking on it now

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

This comment has been minimized.

@mihaic

mihaic Sep 21, 2016
Contributor

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

lang = ext.language or self.compiler.detect_language(ext.sources)
if lang == 'c++':
@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 22, 2016

Jenkins, retest this please.

@mihaic mihaic merged commit e50068d into brainiak:master Sep 22, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@buildbot-princeton
linux Build finished.
Details
@buildbot-princeton
macos Build finished.
Details
@yidawang yidawang deleted the yidawang:fcma branch Sep 22, 2016
danielsuo pushed a commit that referenced this pull request Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants