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

soap_turbo may not be thread safe #7

Open
bernstei opened this issue Jun 2, 2023 · 6 comments
Open

soap_turbo may not be thread safe #7

bernstei opened this issue Jun 2, 2023 · 6 comments

Comments

@bernstei
Copy link

bernstei commented Jun 2, 2023

The use of function-local save variables breaks thread safety, and soap_turbo uses it for W, S, and multiplicity_array in soap_turbo.f90 (line 71).

Has anyone thought about this issue yet? Can anyone explain what those variables are, and why they are save?

@mcaroba
Copy link
Collaborator

mcaroba commented Jun 5, 2023

I have indeed not thought about thread safety. The W and S variables are built during orthogonality construction and are save to avoid having to recompute the radial basis coefficients every time the subroutine is invoked. There is a recompute_basis variable (also save) which is used to recompute the basis whenever subsequent calls to the subroutine use a different basis set (or the basis has not been previously computed). I think this might be enough to ensure thread safety, but I'm not 100% sure.

(S is the matrix of overlap coefficients of the original [non orthonormal] basis, and W is the matrix of linear coefficients to express the orthonormal basis in terms of the original basis)

@bernstei
Copy link
Author

bernstei commented Jun 5, 2023

Thanks. I'll think about it as well.

@bernstei
Copy link
Author

bernstei commented Jun 6, 2023

Are these arrays only dependent on properties of the GAP itself, or do they sometimes need to be recomputed based on something which is only known at compute time, e.g. the specific composition of the configuration?

I.e. if I knew I was only using one GAP+SOAP_turbo potential, could I do all the pre-computation once and count on it not changing during different compute calls?

[I'm thinking of refactoring a bit of it to create a soap_turbo object with a distinct Initialise call, like the potentials, to store this information so there can be two co-existing soap_turbo-based GAPs loaded at the same time, as in what could happen in multithreaded operation]

@mcaroba
Copy link
Collaborator

mcaroba commented Jun 8, 2023

These arrays depend only on:

  • alpha_max for the poly3 basis, so it can actually be tabulated. In fact I recently added a version of this basis with tabulated coefficients.
  • alpha_max and atom_sigma_r for the poly3gauss basis.

They only need to be recomputed if these details change between calls, which will not happen unless the descriptor definition changes.

@bernstei
Copy link
Author

bernstei commented Jun 8, 2023

OK - I may try to make a PR that will create some sort of soap_turbo object that's explicitly initialized, like the potentials, so it can be thread safe. I can even have the argument be optional, so it can fall back to the current behavior and no code that uses soap_turbo has to change unless it wants to be thread safe.

@mcaroba
Copy link
Collaborator

mcaroba commented Jun 8, 2023

OK, sounds good!

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

No branches or pull requests

2 participants