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

Asymmetric correlations (continued from #174 ) #217

Closed
wants to merge 38 commits into from

Conversation

Lazersmoke
Copy link
Contributor

This is #174 but fixed up and rebased onto modern Sunny

Lazersmoke and others added 30 commits January 9, 2024 13:10
Some tests are still erroring and requires more investigation
also updates and upgrades the correlation tests
More specifically, this averages over i and sums over j, instead of summing over both i and j.
@@ -266,6 +266,7 @@ function step!(sys::System{0}, integrator::ImplicitMidpoint)

s = sys.dipoles
(; Δt, atol) = integrator
isnan(Δt) && return
Copy link
Member

Choose a reason for hiding this comment

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

This check is already made in check_timestep_available() above.

@kbarros kbarros changed the title Asymmteric correlations (continued from #174 ) Asymmetric correlations (continued from #174 ) Jan 23, 2024
@Lazersmoke
Copy link
Contributor Author

Here is a list of what this PR adds/changes (can be condensed into some versions.md lines):

  1. Adds negative_energies=true support to the binning interface. (Required to be able to evaluate the sum rule, which requires integrating over [-fs/2,fs/2], and make this test pass)
  2. Corrects the overall scale of intensities returned by intensities_binned to correctly incorporate the bin size. (Required for binned sum rule to be satisfied)
  3. Adds concept of "unilateral_to_bilateral transformation" to the Contractions interface. (Required for agreement between LSWT (=true) and SampledCorrelations (=false). See here for the important part. Amounts to a factor of x2 in intensities in some cases)
  4. Documents/corrects the internal handling of indices of correlations in the contractions interface to be self-consistent.
  5. Adds support function intensities_band_structure, which returns the LSWT intensities in a neat struct bs::BandStructure with bs.dispersion and bs.intensities fields. The idea is that the normalization of these "delta function" intensities requires some verbal explanation (currently located in a comment), which can be given in a docstring for BandStructure later. (also adds show instance for this previously internal type).
  6. Allows querying both (Sx,Sy) and (Sy,Sx) correlations, which are different in general. (Although in certain special cases, such as classical dynamics they coincide up to time reversal/some other symmetry). There is a well-documented test which compares this to a reference time-domain algorithm.
  7. Fixes up the classical intensities calculation by: a. fixing wraparound bug using zero padding, b. inserting a 1/natoms factor, c. replacing an extraneous 1/n_omega with the correct 1/(T - |t|), d. enabling an optional cosine-squared window function which may be useful for systems which do not naturally decorrelate. These changes (correctly) alter the normalization of the intensities, and this change is documented in the test suite. (Required to make the test from (6.) pass)
  8. Optimizes memory allocation when using LSWT-with-kernel
  9. Adds a well-documented test of the "polyatomic sum rule," which is more stringent than existing sum rule tests. The existing "monoatomic" tests still exist and are now better documented

@Lazersmoke
Copy link
Contributor Author

#237 includes points (1) (2) (7b) (7c, sort of) and (9) from the above list

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

kbarros commented Aug 19, 2024

This feature is currently out of scope, but can be revisited if there is sufficient user demand.

@kbarros kbarros closed this Aug 19, 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.

2 participants