Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Changes in b vector order #538

Open
qiaojunfeng opened this issue Jan 20, 2025 · 4 comments · May be fixed by #539
Open

Changes in b vector order #538

qiaojunfeng opened this issue Jan 20, 2025 · 4 comments · May be fixed by #539

Comments

@qiaojunfeng
Copy link
Collaborator

I noticed that the order of b vectors has changed in develop branch, probably due to #533 and #498?

@sjhong6230 Is this indeed an incompatibility change (wrt the old version), to make sure those two PRs work? Thanks!

If so, probably we need to add an entry for this in the v4.0 release announcement.

--- a/gaas.nnkp.old
+++ b/gaas.nnkp.new
@@ -1,20 +1,20 @@
-File written on 20Jan2025 at 19:50:46
+# File written on 20Jan2025 at 19:50:32

 begin nnkpts
    8
-     1     2      0   0   0
-     1     3      0   0   0
-     1     5      0   0   0
-     1     8      0   0   0
-     1     2      0   0  -1
-     1     3      0  -1   0
-     1     5     -1   0   0
-     1     8     -1  -1  -1
-     2     1      0   0   0
-     2     4      0   0   0
-     2     6      0   0   0
-     2     1      0   0   1
-     2     7      0   0   1
-     2     4      0  -1   0
-     2     6     -1   0   0
-     2     7     -1  -1   0
+       1       2      0   0   0
+       1       3      0   0   0
+       1       5      0   0   0
+       1       8      0   0   0
+       1       2      0   0  -1
+       1       3      0  -1   0
+       1       5     -1   0   0
+       1       8     -1  -1  -1
+       2       1      0   0   1
+       2       4      0   0   0
+       2       6      0   0   0
+       2       7      0   0   1
+       2       1      0   0   0
+       2       4      0  -1   0
+       2       6     -1   0   0
+       2       7     -1  -1   0
@sjhong6230
Copy link
Contributor

Yes. Since translation invariance and Stengel-Spaldin functional both sum over k-points before b vectors, the order of b vectors should be the same for all k-points.

Thus, I added the kmesh_sort subroutine, and the b vector order in the benchmark file was changed.

Also, the order of elements for the uHu checkpoint files was chabged.

Please let me know if there are some issues with these orders.

@qiaojunfeng
Copy link
Collaborator Author

Thanks @sjhong6230 for the quick reply!

In theory there is no problem with changing the order, it's just a bit unfortunate that we could not reuse the old mmn files anymore.
Have you deleted the old subroutine for generating b vectors? If not and if it's easy, maybe we could add one input parameter to restore the old behaviour (we could still default to the new order)?
But if this is complex and causing difficulties in maintainability, then it's fine, we just need to announce the changes for the new release.

@sjhong6230
Copy link
Contributor

Restoring the old behavior would be easy since the reordering is done in kmesh_sort subroutine, which is done as a post-processing after kmesh_get. Thus, we only need to add one input parameter to disable/enable this subroutine. I think order_b_vectors would be a good name. (true for the reordering, false for the old behavior)

Also, since Stengel-Spaldin functional and translation-invariance works with only sorted kmesh, we should raise an error if order_b_vectors is false for these cases.

@qiaojunfeng
Copy link
Collaborator Author

Sounds good, with this we can maintain some backward compatibility. Would you be willing to create a PR for this? Thanks!

@sjhong6230 sjhong6230 linked a pull request Jan 23, 2025 that will close this issue
@qiaojunfeng qiaojunfeng linked a pull request Jan 23, 2025 that will close this issue
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants