-
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
Proposed modifications to the interface #86
Conversation
Thanks for this! Most of this looks great. I'll to go over this with a bit more care tomorrow and leave comments in the code where relevant. Some initial thoughts:
|
Re: 4. FYI: it defaults to Agreed that the user shouldn't need to think about the fact that they're passing a function to It may be better to introduce a
|
You both have more experience than me, so I'll be happy to try out whatever you decide. Given how many moving parts there are, it might be good to push a prototype to |
I'm definitely for merging as soon as possible. I think maybe we should tweak some of the naming, but maybe that's best done in conjunction with users. Two things I think should be addressed beforehand.
|
Re: Benchmarking Firstly, the only possible slow-down is with the changes to the internals; My holistic benchmark for this is just to time how long it takes to perform one "forwards problem" for the FeI2 example. Without this PR, it takes 59-60 seconds, and after this PR it takes up to 60.6 seconds. So whatever performance regression there may be with this doesn't impact things enough to worry about in my opinion. |
Even worse, the shape of the calculation of intensities is not the same for classical (SF) vs quantum (SWT) calculations. I'm working on making |
There's no docs for If the docs build, this is OK to be a prototype on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me. I think it would be good to get into main once tests and docs build.
# SQ N.B.: This error doesn't make much sense, does it? | ||
# So what if they used a different number from the default number of observables? | ||
# Case in point: If you are doing dipole correlations in SU(N) mode, you're not taking | ||
# the full trace, and this will error out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm not following. If you're doing dipole correlations in SU(N) mode, it will take the first branch (since sf.dipole_corrs will be true). (The checking for 3 is not necessary, however, when sf.dipole_corrs is true.)
I guess there is ambiguity here in terms of what is meant by "trace." Basically, "trace" means Sxx+Syy+Szz unless the user has specified custom observables/correlations, in which case it means \sum Taa where a runs from 1 to N^2-1.
My feeling was that there should be some check here to make sure your not just summing some random collection of diagonal elements. But I guess we expect a user specifying arbitrary correlations to be sophisticated enough to know what they're doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're in SU(N) mode, you may still have a number different from N*N-1 number of correlations included in your trace, for example if you have M observables, you have M =/= N*N-1 terms.
The code now just directly checks that, for each observable in the structure factor, that observable's autocorrelation is included in the trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the point of the check was precisely to prevent the user from thinking they're getting a trace if M is not equal to the dimension of the Lie algebra su(N), namely N^2-1. This has physical significance. (The check is quick and dirty in the sense that it doesn't check is the N^2-1 operators indeed form an orthonormal basis for NxN Hermitian matrices.)
I'm not sure this should be called Trace
if it's just a sum or some available set of autocorrelations, and I do think there should be a trace option. Maybe Trace
should just mean Sxx+Syy+Szz, which is what most people will expect. If someone wants to do a trace of a full set of SU(N) observables, they'll just have to write their own formula.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code now just directly checks that, for each observable in the structure factor, that observable's autocorrelation is included in the trace.
I guess this is fine too, now that I read it a bit more carefully, since, in the default situation, the user will get Sxx+Syy+Szz. In any case, just explaining the reasoning behind the check.
Extremely open to changes on all of this! This is just to get the ball rolling so we can start landing breaking changes :)
The main changes are:
Changes to the intensities interface:
intensities
renamed tointensities_interpolated
, and optioninterpolation = :none
renamed tointerpolation = :round
kT
,formfactors
, andcontractor
are removed fromintensities
andintensities_binned
intensity_formula(sf, :perp; kT, formfactors)
(documentation reproduced below)do
-notationSupporting changes to the internals:
dipole_trajectory
andexpectation_trajectory
have been combinedLinearMap
s (new dependency). [N.B. the custom dipole operators is a Martin requested feature in disguise]Symbol
s inStructureFactor
in order to facilitate users referring to them (e.g. when specifying correlations to save) without having to use indices all the time. Providing observable names is optional (default alphabetical) and indexing by number is still supported.show(::StructureFactor)
facelift:Sparsity pattern looks better in reality:
Example creating a
SU(2)
system with onlySx
andSz
Documentation for
intensity_formula
(reproduced here for clarity)
Establish a formula for computing 𝒮(𝐪,ω) from the correlation data stored in the
StructureFactor
. The formula returned from intensity_formula can be passed tointensities_interpolated
orintensities_binned
.Sunny has several built-in formulas that can be selected by setting contraction_mode to one of these values:
:trace
, which yields tr𝒮(q,ω)=∑α𝒮αα(q,ω):perp
, which contracts 𝒮αβ(q,ω) with the dipole factor δαβ−qαqβ, returning the unpolarized intensity.:full
, which will return all elements 𝒮αβ(𝐪,ω) without contraction.Additionally, there are keyword arguments providing temperature and form factor corrections:
kT
: If a temperature is provided, the intensities will be rescaled by a temperature- and ω-dependent classical-to-quantum factor. kT should be specified when making comparisons with spin wave calculations or experimental data. If kT is not specified, infinite temperature (no correction) is assumed.formfactors
: To apply form factor corrections, provide this keyword with a vector of FormFactors, one for each unique site in the unit cell. The form factors will be symmetry propagated to all equivalent sites.Alternatively, a custom formula can be specified by providing a function intensity = f(q,ω,correlations) and specifying which correlations it requires:
The function is intended to be specified using do notation. For example, this custom formula sums the off-diagonal correlations:
If your custom formula returns a type other than
Float64
, use thereturn_type
keyword argument to flag this.Modified keyword arguments to
StructureFactor
observables
: Allows the user to specify custom observables. The observables must be given as a list of complex N×N matrices orLinearMap
s. It's recommended to name each observable, for example:observables = [:A => a_observable_matrix, :B => b_map, ...]
. By default, Sunny uses the 3 components of the dipole,:Sx
,:Sy
and:Sz
.correlations
: Specify which correlation functions are calculated, i.e. which matrix elements αβ of Sαβ(q,ω) are calculated and stored. Specified with a vector of tuples. By default Sunny records all auto- and cross-correlations generated by all observables. To retain only the xx and xy correlations, one would setcorrelations=[(:Sx,:Sx), (:Sx,:Sy)]
orcorrelations=[(1,1),(1,2)]
.