Skip to content

Fix: symmetrize charge density in reciprocal space in any case#3081

Merged
dyzheng merged 7 commits into
deepmodeling:developfrom
maki49:symfixrecip
Oct 26, 2023
Merged

Fix: symmetrize charge density in reciprocal space in any case#3081
dyzheng merged 7 commits into
deepmodeling:developfrom
maki49:symfixrecip

Conversation

@maki49

@maki49 maki49 commented Oct 19, 2023

Copy link
Copy Markdown
Collaborator

@dyzheng dyzheng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree with this Fix code, will the function "psymm" not be used any more due to the new symmetry algorithm?

@maki49

maki49 commented Oct 19, 2023

Copy link
Copy Markdown
Collaborator Author

I agree with this Fix code, will the function "psymm" not be used any more due to the new symmetry algorithm?

Currently yes, but I think it's better to keep it in case of future usage.
Should I comment out the implementation too for unit test coverage?

@pxlxingliang pxlxingliang left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi, @maki49 please add an integrate test for symmetry. Suggest using the structure in issue #3070 and using the results of symmetry = 0 as the reference, and please also modify the INPUT to reduce the running time (preferably around 10 seconds)

@hongriTianqi hongriTianqi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The number of warnings in CI test increased from 56 to 87. Please treat the CI case reference if needed.

@maki49

maki49 commented Oct 20, 2023

Copy link
Copy Markdown
Collaborator Author

The number of warnings in CI test increased from 56 to 87. Please treat the CI case reference if needed.

To avoid warnings will be conflict with the demand of @pxlxingliang : if results with symmetry=0 are used as the reference to run test cases with symmetry=1, the warnings will be inevitable unless loosing the threshold.

@hongriTianqi

Copy link
Copy Markdown

The number of warnings in CI test increased from 56 to 87. Please treat the CI case reference if needed.

To avoid warnings will be conflict with the demand of @pxlxingliang : if results with symmetry=0 are used as the reference to run test cases with symmetry=1, the warnings will be inevitable unless loosing the threshold.

Sorry, I don't understand why the warnings will be inevitable.

@maki49

maki49 commented Oct 20, 2023

Copy link
Copy Markdown
Collaborator Author

Sorry, I don't understand why the warnings will be inevitable.

The magnitude of the energy difference between symmetry=0 and symmetry=1 could be around 1e-3 Ry due to the charge density symmetrization. See some results I've tested before: https://xmywuqhxb0.feishu.cn/docx/MAAadv569onfU0xtbc5cEFwjnRh?from=from_copylink

@dyzheng

dyzheng commented Oct 20, 2023

Copy link
Copy Markdown
Collaborator

The number of warnings in CI test increased from 56 to 87. Please treat the CI case reference if needed.

To avoid warnings will be conflict with the demand of @pxlxingliang : if results with symmetry=0 are used as the reference to run test cases with symmetry=1, the warnings will be inevitable unless loosing the threshold.

I suggest to change reference from "symmetry=0" to "symmetry=1" in these cases.
I have tested QE for low ecutrho, there is inevitable error between symmetry on and off.

@maki49

maki49 commented Oct 23, 2023

Copy link
Copy Markdown
Collaborator Author

I wonder why 902_OF_KE_WT timed out on CI...
I failed to reproduce it on both my workstation and Bohrium, complied by both Intel and GNU.

@hongriTianqi

Copy link
Copy Markdown

I wonder why 902_OF_KE_WT timed out on CI...
I failed to reproduce it on both my workstation and Bohrium, complied by both Intel and GNU.

It's because the total time limit for case tests are 1500 seconds.

@dyzheng

dyzheng commented Oct 23, 2023

Copy link
Copy Markdown
Collaborator

I wonder why 902_OF_KE_WT timed out on CI... I failed to reproduce it on both my workstation and Bohrium, complied by both Intel and GNU.

Some cases (such as 204_NO_KP_AFM : 110 s vs 17 s) used much more time than before.
I have tested this case with your code changes, it can't convergent in SCF after changes.

develop branch :
image

This PR:
image

@maki49 the result of develop branch is same as symmetry=-1 !

@dyzheng dyzheng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please fix the bugs in CI

@maki49

maki49 commented Oct 23, 2023

Copy link
Copy Markdown
Collaborator Author

@maki49 the result of develop branch is same as symmetry=-1 !

Is it possible that it's a convergence to a more stable state (with lower energy and smaller stress) rather than a bug? (I'm not sure, maybe more tests are needed...)
After I merged #3080 into this PR, convergence to the same result (-6.438311e+03eV, -173.86 kbar) just needs 61 steps:
symm1_3080.log

Another point: in LCAO cases, a round trip of FFT can cause a loss of precision. Here's the log that I only do a round trip of FFT without any charge density symmetrization:
fft_only.log

A possible solution is:

  • for PW, do the symmetrization in reciprocal space in any case (as this change)
  • for LCAO, do the symmetrization in real space for the pure point group and in reciprocal space for other cases (as before)

Since the LCAO-FFT problem is particular in ABACUS (that we cannot refer to VASP and QE), I think it better to leave it an open question for further tests and discussion, with no need to block merging this PR for current usage.
If the error cases are calculated on PW basis, what do you think of the above solution? @dyzheng @pxlxingliang

@dyzheng

dyzheng commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator

Is it possible that it's a convergence to a more stable state (with lower energy and smaller stress) rather than a bug? (I'm not sure, maybe more tests are needed...)

I don't agree your point, the critical point is this case is anti-ferromagnetic material, I tested "ecutwfc 100" and KPT: 6 6 6 for both "nspin 1" and "nspin 2".
Results of "nspin 1" are same among symmetry=-1, 0, and 1.
But results of "nspin 2" are same between symmetry = -1, 0, and different with symmetry = 1.

You can test by yourself~

@maki49

maki49 commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator Author

I don't agree your point, the critical point is this case is anti-ferromagnetic material, I tested "ecutwfc 100" and KPT: 6 6 6 for both "nspin 1" and "nspin 2". Results of "nspin 1" are same among symmetry=-1, 0, and 1. But results of "nspin 2" are same between symmetry = -1, 0, and different with symmetry = 1.

You can test by yourself~

You're right. This problem arises from the loop of primitive cells in supercell during reciprocal-space charge density symmetrization. The primitive cell analysis doesn't consider atom magmoms, which gives more primitive cells than the right answer.

  • Current fix: if not all the atoms in its type has the same magmom, do not traverse primitive cells in rhog_symmetry. (This way may lead to a possible incomplete symmetrization due to not exploiting all the primitive-cell-translational symmetry, but it can give the right result.)
  • Perfect solution In the future: spglib can be used as an option to analyze magnetic space group.

@maki49 maki49 marked this pull request as draft October 24, 2023 14:19
@maki49 maki49 marked this pull request as ready for review October 25, 2023 17:00
@dyzheng

dyzheng commented Oct 26, 2023

Copy link
Copy Markdown
Collaborator

Thanks for your excelIent work for fixing symmetry bugs!
I suggest to merge this PR as soon as possible.
I hope any volunteer who will be interested to add more unit tests to cover every case of symmetry in the future.

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

Successfully merging this pull request may close these issues.

URGENT: Inconsistent stress and force calculated by ABACUS and QE, resulting different even unreasonable structures

4 participants