-
Notifications
You must be signed in to change notification settings - Fork 19
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
Update/correct normalizations #237
Conversation
b4ea68b
to
beff7bc
Compare
This brings the normalization back into agreement with the structure factor doc
I just want to make sure I get the basic point. By changing the |
Hi @ddahlbom , thanks for the question. There needs to be a division by the number of estimates to compute the correlation (because mean=sum/count). So, previous to wraparound bug fix, it would be 1/nomega, but after wraparound bug fix, it would be 1/|t-T| instead. |
(and separately to this, there's a number prod(latsize) of spatial estimates that also gets divided out) |
While I'm thinking about it, let me record this slack message here, which details a particular working configuration that gets you the sum rule:
(N.B.: those two bugs are fixed in 8e10686 )
N.B.: that script includes a factor two which we still need to understand. It's on this line: |
All fixes from this PR have been incorporated into PR #246. |
Don't merge this yet please; adding things in stages so the commits can be cherry picked if needed!Update: Ready to merge as soon as tests pass!
Might want to write some
versions.md
lines to the tune of:Computes the mean correlation (previously computed mean of correlations-summed-over-unit-cell, which is what SpinW does). Concretely: introduces a[KB: I think this change is now reverted, per comment below]1/natoms
factor1/nomega
factorintensities_binned
normalization to correctly integrate correlation intensities over the bins