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

Fixes #1054 by ensuring thread savety of hessian calculations with GFN-FF #1061

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

Thomas3R
Copy link
Contributor

@Thomas3R Thomas3R commented Jul 2, 2024

The distance array neigh%distances was not allocated thread save and was removed from the neigh class.
Now the distances are calculated from the atom coordinates and the translation vectors.

@Thomas3R Thomas3R requested review from marcelmbn and Albkat July 2, 2024 11:58
@marcelmbn marcelmbn added the bug Something isn't working label Jul 2, 2024
@marcelmbn
Copy link
Member

Given the assumption that the distance array cannot be allocated thread safe, this seems like a reasonable change to me.
However, it's principle-wise not a good development to move away from modularized calculation of static variables (like a distance array) towards calculating it from scratch whenever needed.
Thus, do you think there's any chance to allocate it in a thread-safe way?

@Thomas3R
Copy link
Contributor Author

Thomas3R commented Jul 2, 2024

Of course, a distance array could be used in a thread-saving way. However, I recommend implementing this together with a revision of the force field parallelization. This would be a major effort to implement either way.

Copy link
Member

@marcelmbn marcelmbn left a comment

Choose a reason for hiding this comment

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

Again, assuming that there's no better solution using the distances array of the neigh type in a thread-safe manner, this seems like a reasonable change.

@Thomas3R Thomas3R merged commit edcfbbe into grimme-lab:main Jul 2, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants