search

Xtra-Computing / thundersvm

ThunderSVM: A Fast SVM Library on GPUs and CPUs
Apache License 2.0
1.55k stars 215 forks source link

Add support for external Eigen3 #196

Closed emmenlau closed 4 years ago

emmenlau commented 4 years ago

This PR is based on #195. It adds support for an external Eigen3 library. If a recent version of Eigen3 is already installed, it will provide a cmake package configuration file. If this exists for at least Eigen version 3.3, it is used instead of the internal Eigen3.

emmenlau commented 4 years ago

This PR seems to have a build error for macOS on Travis. However I think the code is correct, because it works for me locally on macOS. Could you also take a look?

emmenlau commented 4 years ago

This PR seems to have a build error for macOS on Travis. However I think the code is correct, because it works for me locally on macOS. Could you also take a look?

I have found a better way to generally set Eigen includes and will update the PR accordingly.

Is it correct that either Eigen3 or CUDA is used, never both at the same time?

zeyiwen commented 4 years ago

Yes. You are correct. Only one of them is used, and never both at the same time.

emmenlau commented 4 years ago

Yes. You are correct. Only one of them is used, and never both at the same time.

Thanks @zeyiwen . I have added a dedicated check for this, because USE_EIGEN enables -march=native which is otherwise not added. Currently it is possible to set USE_EIGEN and USE_CUDA at the same time, which this PR excludes, to make sure the CUDA build is always consistent.

I hope that is good and in your interest?

emmenlau commented 4 years ago

This PR seems to have a build error for macOS on Travis. However I think the code is correct, because it works for me locally on macOS. Could you also take a look?

I have found a better way to generally set Eigen includes and will update the PR accordingly.

This is fixed now. In the current status, Eigen support should now work correctly with external or internal Eigen3. Tested on Linux, macOS and Windows.

Please review and let me know what you think?

zeyiwen commented 4 years ago

Thanks @emmenlau Is it possible not to have cmake/thundersvmConfig.cmake.in for simiplification?

emmenlau commented 4 years ago

Is it possible not to have cmake/thundersvmConfig.cmake.in for simiplification?

Its possible. But the file can significantly help users to work with thundersvm in downstream projects. It works very well with cmake and all the tools based on it (like conan). The reason to add the file is so that downstream projects can find the same requirements as thundersvm, like OpenMP, Eigen, CUDA etc. Without this file, downstream projects need to add these dependencies manually before doing

find_package(thundersvm)

but since they may not know how thundersvm was configured this becomes a bit arbitrary and hard.

zeyiwen commented 4 years ago

I have resolved the conflicts due to the previous PRs, and now merged into ThunderSVM. Would you please help test/double-check if the latest version works on your end?

emmenlau commented 4 years ago

Would you please help test/double-check if the latest version works on your end?

Thanks @zeyiwen , I confirm everything works very well! We've just implemented preliminary macOS support, and it only requires setting the path to the includes. I'll create a separate PR soon.