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

mnt: changed spin_squared name to other name #497

Merged
merged 3 commits into from
Mar 8, 2024
Merged

mnt: changed spin_squared name to other name #497

merged 3 commits into from
Mar 8, 2024

Conversation

zerothi
Copy link
Owner

@zerothi zerothi commented Oct 12, 2022

The correct name is a contamination of the spin states. This change adheres so that it is more correct.

@tfrederiksen @sofiasanz could you please review this name change?
I think this is more correct, no? You have it used in hubbard, but just wanted to clear it out.

  • Tests added
  • Documentation for functionality
  • User visible changes (including notable bug fixes) are documented in CHANGELOG

@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 86.76%. Comparing base (2334b58) to head (e34dd38).

Files Patch % Lines
src/sisl/physics/electron.py 91.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #497      +/-   ##
==========================================
- Coverage   86.76%   86.76%   -0.01%     
==========================================
  Files         399      399              
  Lines       50548    50544       -4     
==========================================
- Hits        43860    43854       -6     
- Misses       6688     6690       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zerothi zerothi added this to the v0.13.0 milestone Oct 12, 2022
@zerothi
Copy link
Owner Author

zerothi commented Oct 26, 2022

Hmm, I stumbled upon this one: https://arxiv.org/pdf/1508.02058.pdf

There seems to be more terms that could be useful?

@tfrederiksen
Copy link
Contributor

tfrederiksen commented Nov 4, 2022

I agree to choose the standard name of spin_contamination. However, the returned quantity is not exactly the single number that some people may be used to. For instance, this is the typical spin contamination block reported by ORCA:

UHF SPIN CONTAMINATION
----------------------

Warning: in a DFT calculation there is little theoretical justification to
         calculate <S**2> as in Hartree-Fock theory. We will do it anyways
         but you should keep in mind that the values have only limited relevance

Expectation value of <S**2>     :     0.767859
Ideal value S*(S+1) for S=0.5   :     0.750000
Deviation                       :     0.017859

Anyways, as long as the documentation is clear what is calculated, I guess the user can figure things out from there.

@tfrederiksen
Copy link
Contributor

Maybe one could have an optional keyword to take the calculation one step further to compute the three values above?

@zerothi
Copy link
Owner Author

zerothi commented Nov 5, 2022

Maybe one could have an optional keyword to take the calculation one step further to compute the three values above?

Yes, I think that perhaps it should default to all terms given in the article I reference, then options could be used to query them individually. However the current form returns all contamination values for a single state, whereas the article (and orca) seems to only return single numbers, what are your thoughts about that? Should we have a keyword for sum=true/false?

@tfrederiksen
Copy link
Contributor

I think sum=True would be a reasonable default. While the state-resolved quantity may be of some relevance for expert users, I think the general idea of spin contamination is to characterize with a single number how the calculated many-electron state, i.e., the single Slater determinant in mean-field approaches, deviates from a true eigenstate of the S^2 operator. Hence, the sum over filled states would be natural.

Regarding the terms in your reference (which I didn't study), they are only relevant for noncollinear and spin-orbit calculations, right? I think what you already have is the "standard" expression for spin contamination (up to the exact S^2 value and N_beta).

@zerothi
Copy link
Owner Author

zerothi commented Nov 5, 2022

Thanks, yes it covers nc/soc, but also discusses the collinear term.

@zerothi zerothi modified the milestones: v0.13.0, v0.14 Jan 11, 2023
@zerothi
Copy link
Owner Author

zerothi commented Feb 16, 2024

I think sum=True would be a reasonable default. While the state-resolved quantity may be of some relevance for expert users, I think the general idea of spin contamination is to characterize with a single number how the calculated many-electron state, i.e., the single Slater determinant in mean-field approaches, deviates from a true eigenstate of the S^2 operator. Hence, the sum over filled states would be natural.

Regarding the terms in your reference (which I didn't study), they are only relevant for noncollinear and spin-orbit calculations, right? I think what you already have is the "standard" expression for spin contamination (up to the exact S^2 value and N_beta).

Could you remind me here, should sum=True sum everything, both, so sum(Sa) + sum(Sb)?
I will leave the rest for a later round.

@@ -7,7 +7,7 @@
from sisl._internal import set_module

from .distribution import get_distribution
from .electron import EigenstateElectron, EigenvalueElectron, spin_squared
from .electron import EigenstateElectron, EigenvalueElectron

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
sisl.physics.electron
begins an import cycle.
@zerothi zerothi modified the milestones: v0.14, v0.15 Mar 6, 2024
zerothi added 2 commits March 8, 2024 08:42
The correct name is a contamination of the spin states.
This change adheres so that it is more correct.
Signed-off-by: Nick Papior <[email protected]>
@zerothi
Copy link
Owner Author

zerothi commented Mar 8, 2024

I think it was obvious once I looked into it, will now default to return the sum (sum(Sa)).

Added `sum` flag for returning the sum of the
spin_contamination.

Signed-off-by: Nick Papior <[email protected]>
@zerothi zerothi merged commit a50e343 into main Mar 8, 2024
8 checks passed
@zerothi zerothi deleted the spin-fix branch March 8, 2024 08:35
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.

2 participants