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

Parallelization via OpenMP support #6

Merged
merged 3 commits into from
Nov 10, 2022
Merged

Conversation

Abde5
Copy link

@Abde5 Abde5 commented Oct 31, 2022

Dear Wei, Liu, Ling and Su,

Reading the Discussion chapter of your paper, we took the freedom to implement a parallelization of CoACD.

We did so by parallelizing part of the Compute problem (each CoACD iteration is perfectly parallelizable) and the merge (the cost matrix computing has been parallelized). We attempted to implement a more complex parallelization system with OpenMP tasks; however, it did not give good results compared to a basic parallelization of the for routines.

The Merge loop has been simplified in order to keep it a single loop.

With empirical tests, we have observed that we achieve speedups from 1.2 to 3.8 for the smaller concavity thresholds.

chrome_yjWLeCmtUX

Please be free to try it out and I would be glad to answer any questions about the implementation.

This work is being contributed by Altheria Solutions.

Copy link
Contributor

@olitheolix olitheolix left a comment

Choose a reason for hiding this comment

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

@Abde5 It did not compile on Linux out of the box due to various

.../process.cpp:229:37: error: ‘pmeshs’ not specified in enclosing ‘parallel’

errors. However, I managed to make them disappear with the two suggested changes. CoACD now uses all cores during the decomposition phase of the algorithm.

Thank you for that PR 🚀

src/process.cpp Outdated Show resolved Hide resolved
src/process.cpp Outdated Show resolved Hide resolved
@Abde5
Copy link
Author

Abde5 commented Nov 4, 2022

@olitheolix to be honest, the PR did not even compile on Windows 😅

I didn't PR with the latest commit in my local branch, thanks for fixing the problem, tho!

@SarahWeiii
Copy link
Owner

Hi @Abde5 ,
Thank you for the contribution, I have tested the parallelization on Ubuntu 20.04 and will merge it into the main branch.

@SarahWeiii SarahWeiii merged commit a987bbf into SarahWeiii:main Nov 10, 2022
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.

None yet

4 participants