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

Remove unused dependency OpenMP #26

Closed
wants to merge 1 commit into from

Conversation

siggmo
Copy link
Collaborator

@siggmo siggmo commented Oct 4, 2024

While working on #25, I stumbled across the OpenMP dependency in the CMakeLists.txt file. It was not covered by our listed dependencies when I tried to install them with brew on macOS. So I wanted to check if it was even needed, and it actually isn't. Building FANS as well as all test cases succeed without OpenMP. I guess I confused it with openmpi in the first place, since I'm not really familiar with both.

This PR removes OpenMP as a dependency in the CMake recipe.

Copy link
Collaborator

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @sanathkeshav has some plans to include shared memory parallelization using OpenMP, and hence he probably kept the dependency.

@sanathkeshav
Copy link
Collaborator

Hi Moritz!

Thanks for the PR. You are absolutely right that OpenMP is currently not used at all.
But I think you included it in the CMake recipe because I had included it in the old recipe.

As @IshaanDesai mentioned, Yes I plan to use it in the near future so we should leave it as is at the moment.

For some extra context: FFTW allows for MPI parallelism via slab domain decomposition only. So within a slab, we would like to introduce shared memory parallelism via OpenMP.

@siggmo
Copy link
Collaborator Author

siggmo commented Oct 7, 2024

Okay then let's add it to the installation instructions as well, right? At least on the macOs runner it was not preinstalled and thus the CMake recipe failed.

@IshaanDesai
Copy link
Collaborator

Okay then let's add it to the installation instructions as well, right? At least on the macOs runner it was not preinstalled and thus the CMake recipe failed.

We can add this directly on develop.

@IshaanDesai IshaanDesai closed this Oct 7, 2024
@sanathkeshav sanathkeshav deleted the bugfix/remove-openmp-dependency branch October 28, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants