mtazzari / galario

Gpu Accelerated Library for Analysing Radio Interferometer Observations
https://mtazzari.github.io/galario/
GNU Lesser General Public License v3.0
31 stars 15 forks source link

Move away from C interface to throw exceptions #125

Closed fredRos closed 6 years ago

fredRos commented 6 years ago

Needed to have exceptions #25

fredRos commented 6 years ago

Re test failure: I scratched my head for an hour today and I have no idea why it fails on travis. Works fine on my machine, there seems to be a weird mixup about using the cython imported numpy instead of the python numpy. But why this happens on this branch, I will have to figure out tomorrow...

fredRos commented 6 years ago

I tested that all tests pass with cuda 9.1 on a V100. @mtazzari please check the code on your cuda powered machine when you find the time to do so. I remove the [wip] tag, so it's ready to go into master from my side. But since the changes are substantial, take your time to play with the code

fredRos commented 6 years ago

Oops, I just realized I didn't update the docs accordingly. [wip] is back again

mtazzari commented 6 years ago

Try a fit on GPU with very large matrices and/or many processes to check if the exceptions are thrown.

mtazzari commented 6 years ago

There are a lot of conflicts. I would like to test this once we merged PR #133 Could you have a look into that?

mtazzari commented 6 years ago

In the meanwhile, I have fetched the branch from your repo. I am trying to compile on my desktop but I get

[  8%] Built target lookup_dependencies
[  8%] Building CXX object src/CMakeFiles/galario_single.dir/galario.cpp.o
[  8%] Building CXX object src/CMakeFiles/galario.dir/galario.cpp.o
[  8%] Built target tests
[ 13%] Linking CXX shared library libgalario_single_cuda.so
[ 17%] Linking CXX shared library libgalario_cuda.so
[ 21%] Built target galario_single_cuda
[ 30%] Built target galario_cuda
[ 39%] Compiling Cython CXX source for libcommon_single_cuda...
[ 39%] Compiling Cython CXX source for libcommon_cuda...
/home/mtazzari/repos/galario/src/galario.cpp:71:25: error: expected type-specifier
/home/mtazzari/repos/galario/src/galario.cpp:71:25: error: expected ‘>’
/home/mtazzari/repos/galario/src/galario.cpp: In function ‘void galario::init()’:
/home/mtazzari/repos/galario/src/galario.cpp:318:81: error: no matching function for call to ‘throw_exception(const char [45], int, const char [5], const char [27])’
/home/mtazzari/repos/galario/src/galario.cpp:318:81: note: candidate is:
/home/mtazzari/repos/galario/src/galario.cpp:72:10: note: template<class T> void {anonymous}::throw_exception(const char*, int, const char*, const string&)
/home/mtazzari/repos/galario/src/galario.cpp:72:10: note:   template argument deduction/substitution failed:
/home/mtazzari/repos/galario/src/galario.cpp: In function ‘dcomplex* galario::copy_input(int, int, const dreal*)’:
/home/mtazzari/repos/galario/src/galario.cpp:379:5: error: ‘invalid_argument’ is not a member of ‘std’
/home/mtazzari/repos/galario/src/galario.cpp:379:5: error: ‘invalid_argument’ is not a member of ‘std’
/home/mtazzari/repos/galario/src/galario.cpp:379:5: error: no matching function for call to ‘throw_exception(const char [45

etc... I am using gcc/g++ 4.7.2. Any idea?

[UPDATE] With gcc?g++ 5.3.0 it works perfectly. We need to boost the gcc/g++ requirements in the docs, and perhaps in the Cmake.

fredRos commented 6 years ago

I am using gcc/g++ 4.7.2. Any idea?

Yeah that's too old. GCC 4.8.1 was the first to fully support c++11; cf. https://gcc.gnu.org/projects/cxx-status.html It looks like we have been using C++11 features but didn't actually require it in cmake and didn't explain in the installation instructions. I need to fix that, have done similarly for other projects

fredRos commented 6 years ago

Ok, we have been using c++11 like this:

set(cxx_std "-std=c++11")
set(CMAKE_CXX_FLAGS "${cxx_std} ${CMAKE_CXX_FLAGS}")

But that means older compilers that only have partial support for c++11 won't fail here because they accept the flag -std=c++11. I tested compilation with g++-4.9 and got an error related to exception that is easily fixed by including the appropriated <stdexcept> header.

This discussion has several approaches with pros and cons on requiring a compiler

fredRos commented 6 years ago

I fixed up your check for dR to use a macro. In addition I cleaned up the c++ build a bit. I will now work on the memory deallocation. Looks like there is no way around try...catch unless we rewrite everything to have wrappers with destructors around the cuda arrays. Someone else has done the work for us already. I need to evaluate if try catch is simpler (also long-term), or if it's worth refactoring to use this cuda C++ wrapper https://github.com/eyalroz/cuda-api-wrappers https://codedocs.xyz/eyalroz/cuda-api-wrappers/unique__ptr_8hpp.html

mtazzari commented 6 years ago

The introduction of CudaMemory makes the code so much cleaner and readable, and also more elegant. We save a lot of lines of code for allocating, copying H2D and D2H, as well as many nbytes definitions. Great job! Looking forward to merge!