Update/correct normalizations

Lazersmoke commented 5 months ago

~~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 1/natoms factor~~ [KB: I think this change is now reverted, per comment below]
Makes frequency axis consistent with wavevector axes by removing an extra 1/nomega factor
Updates intensities_binned normalization to correctly integrate correlation intensities over the bins

Lazersmoke commented 4 months ago

With 61aad31, this branch now implements (for the classical dynamics) the normalizations described in the #240 docs.

(It still has wraparound bug on this branch--but it's a correctly normalized wraparound bug :) )

ddahlbom commented 2 months ago

I just want to make sure I get the basic point. By changing the nomegaportion of the normalization, how does this affect the sum rule calculations practically? I assume it will become necessary to divide by nomega (roughly, the trajectory length) at the end, just as it is currently necessary to divide by the number of sites (in the case of having a single site per unit cell). Is the point then just maintaining consistency in the way that space and time are normalized, or is there some deeper rationale? (Not asking because I object to the change. Just want to make sure I understand what's going on.)

Lazersmoke commented 2 months ago

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.

Lazersmoke commented 2 months ago

(and separately to this, there's a number prod(latsize) of spatial estimates that also gets divided out)

Lazersmoke commented 2 months ago

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:

The 1/(T-t) is in place of any 1/nw prefactor. In order to get the agreement, you need to use the correct normalizations, which are available in PR #237. [That is, you need to apply both #237 AND #246, keeping normalizationFactor = 1/√(prod(sys.latsize)) during the merge conflict. This is what the branch that I mainly use[1] does]

(actually that PR #237 has two bugs about the intensities_binned prefactor which I need to fix still: it should use abs(det(...)) rather than det to avoid negative intensities in some cases; and it should be using the number of available energies instead of numbins[4] of the histogram. The agreement happens once you fix/workaround these bugs)

(N.B.: those two bugs are fixed in 8e10686 )

[1] https://github.com/Lazersmoke/Sunny.jl/tree/temp-non-local-no-wraparound

Concretely: run this script on the above mentioned branch

https://github.com/SunnySuite/SunnyContributed/blob/main/initiatives/Sam-GT/realspace/ll_lswt_limit.jl to get agreement

N.B.: that script includes a factor two which we still need to understand. It's on this line: https://github.com/SunnySuite/SunnyContributed/blob/1bf8363a5b7cb6157ad3f85d7e4c218201276bf9/initiatives/Sam-GT/realspace/ll_lswt_limit.jl#L110

kbarros commented 2 months ago

All fixes from this PR have been incorporated into PR https://github.com/SunnySuite/Sunny.jl/pull/246.

SunnySuite / Sunny.jl

Update/correct normalizations #237