-
Notifications
You must be signed in to change notification settings - Fork 145
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
Translationally-invariant computation for position-related matrix elements #533
Translationally-invariant computation for position-related matrix elements #533
Conversation
This reverts commit 78192d2.
I don't understand why the tests fails for this remote repository. It passed in my local repository. Does anyone know the reason of this discrepancy? |
Hi Seung-Ju! I will check this myself and report back. |
testpostw90_pt_ksliceshc `At line 297 of file ../postw90/postw90.F90 Error termination. Backtrace: |
Hi again! The cause of the failure is an error whose message is this:
Do you recognise this condition? Probably you know whether this should be changed? This error was not visible to you because I had failed to update postw90.x after a change to the error handler. I've made PR into develop for the error handler change. I've pushed the merge of these changes into your work into this repository: https://github.com/JeromeCCP9/wannier90/tree/wigner-seitz and I think this will allow you update your PR so that it will merge, but I have not solved the underlying problem (the test above) that causes the tests to fail. Yours, Jerome |
This was weird to me since the failed tests does not use full translation invariance. However, I got the clue from the above comment. The fix is in line https://github.com/sjhong6230/wannier90_transl_inv/blob/213a9d1af5808e53728d6209a3cd29cf71ab5136/src/postw90/postw90_readwrite.F90#L1008. I added the initialization of local Since this variable was not initialized, it showed different behaviors for my local tests and the remote tests. (My local repository was compiled with intel fortran, but the CI was compiled with gfortran.) Now, the serial tests are good, but the parallel tests are suffering from memory issues. Edited: The memory issue was resolved by allocating |
Dear Seung-Ju, I'm so sorry that you had to fix my bugs! I've been using -finit to set variables to nan, which normally shows up these problems, but I missed this uninitialised one. Thank you so much for your work resolving this issue! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly implemented and minimal set of changes to provide the translationally-invariant functionality. The changes fit the new code structure and the work includes important bug fixes (initialised variable and parallel allocation issues).
Examples and documentation are also provided.
The main differences of this pull request are
For the implementation, one should change the order of Wigner-Seitz minimal distance method and the Fourier transforms. This was done with the help of the following PR: #339.
Another related problem exists. Since the summation over the b vector is performed at the last stage, the number of Fourier transforms needed is multiplied by the number of b vectors, resulting in a large computational time. I resolved this issue by parallelizing the Fourier transform subroutines.
There is also a b vector sorting issue which was present in #498. To resolve this, the
kmesh_sort
subroutine is also called inpostw90
, and the b vector order for some checkpoint files was changed.Corresponding documentation and tutorials were added.