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

Update/correct normalizations #237

Closed
wants to merge 14 commits into from

Conversation

Lazersmoke
Copy link
Contributor

@Lazersmoke Lazersmoke commented Feb 9, 2024

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

This brings the normalization back into agreement with the structure factor doc
@Lazersmoke
Copy link
Contributor Author

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 :) )

@Lazersmoke Lazersmoke mentioned this pull request Mar 8, 2024
@ddahlbom
Copy link
Member

ddahlbom commented May 18, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Lazersmoke
Copy link
Contributor Author

Lazersmoke commented May 18, 2024

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
Copy link
Member

kbarros commented May 18, 2024

All fixes from this PR have been incorporated into PR #246.

@kbarros kbarros closed this May 18, 2024
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.

3 participants