[mlpack-git] [mlpack/mlpack] LSHSearch Parallelization (#700)

[Yannis Mentekidis]

> Ok, great, I am happy to add this. I made a few changes after the merge. 77d098c removes the HAS_OPENMP CMake variable (but not the compiler definition), since it isn't used anywhere. Let me know if that causes any problems; maybe I misunderstood where you are using it. (Note that the line SET(HAS_OPENMP 1) sets the CMake variable, but add_definitions(-DHAS_OPENMP) makes HAS_OPENMP available as a macro in the code.)

Cool, I didn't know this. I thought SET(HAS_OPENMP 1) was the equivalent to `#define HAS_OPENMP 1`. I'm not using the CMake variable - only the C++ macro so this should not be a problem.


>  ca1e3ec and aaefadd are minor style fixes, but the last one, 44c7c35, concerns me a bit; I changed the line avgIndicesReturned = avgIndicesReturned + refIndices.n_elem to avgIndicesReturned += refIndices.n_elem. It worked on my system, and it appears that it worked on AppVeyor, but maybe there is something I overlooked, so let me know if there is. I guess we will find out soon enough if I broke anything like this!

I don't know why I did it like this. I just copied the OpenMP tutorial's code to be certain what I was doing was right - I feared reductions would depend on specific syntax. That's not true, so your fix is equivalent but easier to read.

Thanks for the merge :)


---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/mlpack/mlpack/pull/700#issuecomment-231671712
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mailman.cc.gatech.edu/pipermail/mlpack-git/attachments/20160711/ff2bdf50/attachment.html>