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

Make use of OpenMP optional #1474

Merged
merged 3 commits into from Jul 27, 2018

Conversation

Projects
None yet
3 participants
@rcurtin
Member

rcurtin commented Jul 25, 2018

This fixes #1473, and I'll release mlpack 3.0.3 with this and some other fixes once this is merged.

Basically this just adds a CMake configuration option USE_OPENMP that allows the user to specify whether or not to use OpenMP.

So you can now configure as

$ cmake -DUSE_OPENMP=OFF ..

and OpenMP won't be used.

I also noticed the mlpack paper citation in the readme is out of date, so I updated that too. :)

rcurtin added some commits Jul 25, 2018

Add and document an option for OpenMP support.
Previously, by default, it was always used.  This fixes #1473.
@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jul 25, 2018

Member

@gmanlan ---I also made some simple documentation changes to link to your Windows tutorial from the main README.md, and added links from the Windows tutorial to the others that are around. Let me know if you see any issues from that (commit 4b33dbb).

Member

rcurtin commented Jul 25, 2018

@gmanlan ---I also made some simple documentation changes to link to your Windows tutorial from the main README.md, and added links from the Windows tutorial to the others that are around. Let me know if you see any issues from that (commit 4b33dbb).

@rcurtin rcurtin added this to the mlpack 3.0.3 milestone Jul 25, 2018

if (USE_OPENMP)
find_package(OpenMP)
endif ()
if (OPENMP_FOUND)

This comment has been minimized.

@zoq

zoq Jul 25, 2018

Member

I'm wondering what's OPENMP_FOUND if USE_OPENMP is OFF; undefined? So there is no need to do something like?:

if (USE_OPENMP  AND OPENMP_FOUND)
...
@zoq

zoq Jul 25, 2018

Member

I'm wondering what's OPENMP_FOUND if USE_OPENMP is OFF; undefined? So there is no need to do something like?:

if (USE_OPENMP  AND OPENMP_FOUND)
...

This comment has been minimized.

@rcurtin

rcurtin Jul 26, 2018

Member

OPENMP_FOUND should be an unset variable, and CMake's if() operator will evaluate to false when a if(<variable>) is called for an undefined variable:

https://cmake.org/cmake/help/v3.0/command/if.html

@rcurtin

rcurtin Jul 26, 2018

Member

OPENMP_FOUND should be an unset variable, and CMake's if() operator will evaluate to false when a if(<variable>) is called for an undefined variable:

https://cmake.org/cmake/help/v3.0/command/if.html

This comment has been minimized.

@zoq

zoq Jul 26, 2018

Member

Thanks for the clarification.

@zoq

zoq Jul 26, 2018

Member

Thanks for the clarification.

volume = {14},
pages = {801--805},
year = {2013}
@article{mlpack2018,

This comment has been minimized.

@zoq

zoq Jul 25, 2018

Member

Nice!

@zoq

zoq Jul 25, 2018

Member

Nice!

This comment has been minimized.

@rcurtin

rcurtin Jul 26, 2018

Member

Yeah, I forgot to do this earlier, sorry about that. :)

@rcurtin

rcurtin Jul 26, 2018

Member

Yeah, I forgot to do this earlier, sorry about that. :)

@gmanlan

This comment has been minimized.

Show comment
Hide comment
@gmanlan

gmanlan Jul 26, 2018

Contributor

Looks good to me!

Contributor

gmanlan commented on 4b33dbb Jul 26, 2018

Looks good to me!

@zoq

zoq approved these changes Jul 26, 2018

Looks great to be, no more comments from my side.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jul 27, 2018

Member

Sounds good, since it is approved and it is such a simple fix I will merge it now.

Member

rcurtin commented Jul 27, 2018

Sounds good, since it is approved and it is such a simple fix I will merge it now.

@rcurtin rcurtin merged commit e723e47 into mlpack:master Jul 27, 2018

5 checks passed

Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rcurtin rcurtin deleted the rcurtin:optional-openmp branch Jul 27, 2018

robertohueso added a commit to robertohueso/mlpack that referenced this pull request Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment