Parallelize compute_esp_over_grid #2891
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
JonathonMisiewicz
left a comment
JonathonMisiewicz
left a comment
There was a problem hiding this comment.
One minor comment.
psi4/src/psi4/libmints/oeprop.cc
Outdated
| outfile->Printf("\n Electrostatic potential to be computed on the grid and written to grid_esp.dat\n"); | ||
| } | ||
|
|
||
Contributor
Author
There was a problem hiding this comment.
@JonathonMisiewicz I accidentally marked this as resolved, please let me know if additional changes are needed for approval of this PR.
Co-authored-by: Jonathon Misiewicz <[email protected]>
loriab
approved these changes
Apr 1, 2023
Member
loriab
left a comment
loriab
left a comment
There was a problem hiding this comment.
Thanks for the parallelization!
Contributor
Author
|
Thanks for reviewing @loriab @JonathonMisiewicz ! |
JonathonMisiewicz
approved these changes
Apr 3, 2023
susilehtola
reviewed
Apr 3, 2023
| } | ||
|
|
||
| #ifdef _OPENMP | ||
| #pragma omp parallel for schedule(dynamic) |
Member
There was a problem hiding this comment.
schedule(guided) would be better
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There was a race condition in the parallel calculation of ESP over grid in memory. This was fixed in PR #1900.
This made the computation of ESP serial and quite slow (as mentioned in #1614 for example).
This PR brings back the OpenMP parallelization, the race condition is avoided by allowing each thread to hold its own copy of the
ElectrostaticIntobject. Thanks to @JonathonMisiewicz for the initial hint.User API & Changelog headlines
Dev notes & details
<ElectrostaticInt>and<Matrix>objects for computing the ESP at a given grid point#pragma omp parallel for schedule(dynamic)to parallelize the outer loop over grid pointsQuestions
cubepropuses a scheme different from the one used byoepropto compute ESP on a grid. I am not sure this is documented. Should it be?compute_esp_over_gridfunction which reads the grid fromgrid.datfile. The way it is designed now, the grid is read iteratively, so one grid point read->one ESP point computed->one ESP point written to the outputgrid_esp.datfile. This is not parallelizible. However, if one would read grid points in batches - this can be parallelized. What do you think?Checklist
Status