General Observables 2

[kbarros]

This rebases and squashes the commits from https://github.com/SunnySuite/Sunny.jl/pull/156. I also fixed some errors and cleaned up some style. Let's use this format for future PRs (a minimal number of squashed PRs on top of main). For PRs, always use git rebase and not git merge to make it easier to read the changes.

[kbarros]

@Lazersmoke can you please take a look at the failing Pyrochlore test at the bottom of test_lswt.jl. It is just the intensities check that is failing, and this is due to a shape error in the array comparison. https://github.com/SunnySuite/Sunny.jl/blob/e4b261a1407fafede82564c242734fdb972a1595/test/test_lswt.jl#L464

[kbarros]

General comment: Does this PR introduce a user interface change? If so, where can I find examples of specifying general observables? Are there examples included in the tests?

[kbarros]

General comment (not urgent): The spacing conventions deviate from the BlueStyle. For example, please use this: f(x, y; z=2). Also, we can use g(; z) instead of g(; z=z). Once all the outstanding PRs get merged, we can consider setting up JuliaFormatter to run as a Github action.

[Lazersmoke]

General comment: Does this PR introduce a user interface change? If so, where can I find examples of specifying general observables? Are there examples included in the tests?

The whole UI change is this:

https://github.com/SunnySuite/Sunny.jl/blob/e4b261a1407fafede82564c242734fdb972a1595/src/SpinWaveTheory/SpinWaveTheory.jl#L37

This constructor doesn't have a doc string, but the struct does. Is that legal? In any case this isn't documented or tested[1] for SpinWaveTheory yet. But the docs are already duplicated for dynamical_correlations and instant_correlations: https://github.com/SunnySuite/Sunny.jl/blob/e4b261a1407fafede82564c242734fdb972a1595/src/SampledCorrelations/SampledCorrelations.jl#L121-L131

so we should find a way to de-duplicate this, ideally. One option is to document and/or export parse_observables (which is the shared internal function which parses the multiple allowed formats of the observables and correlations kwargs into an ObservableInfo) https://github.com/SunnySuite/Sunny.jl/blob/e4b261a1407fafede82564c242734fdb972a1595/src/Operators/Observables.jl#L31

[1] Re: testing. The (default) dipole observables are simply custom observables, so in some sense the whole thing is already being tested, as long as we believe that the dipole observables aren't special enough to break things. Perhaps a test case could use a non-hermitian observable, which is a case not covered by dipole observables.

[Lazersmoke]

In case it's not clear, the supported kwarg inputs to the internal function parse_observables are:

Allowed formats for observables:

• observables = [randn(3,3), randn(3,3), ...] gets named :A, :B, ...
• observables = [:obsOne => randn(3,3), :obsTwo => randn(3,3), ...] for custom names (recommended)
• observables = nothing (default) yields :Sx, :Sy, :Sz dipole observables

Allowed formats for correlations:

• correlations = [(1,1),(3,4),...] yields correlations (:A,:A), (:C,:D), ...
• correlations = [(:A,:A),(:C,:D),...] yields correlations (:A,:A), (:C,:D), ...
• correlations = nothing (default) yields all correlations (upper diagonal)

[kbarros]

Thanks for fixing the bug and clarifying the UI. I will think about the docs question too.

[ddahlbom]

One option is to document and/or export parse_observables (which is the shared internal function which parses the multiple allowed formats of the observables and correlations kwargs into an ObservableInfo)

I suppose parse_observables could just be changed into the constructor for ObservableInfo, and the user would be expected to make an ObservableInfo just as they make a formula. But that does make a public interface commitment, which we may wish to shy away from at the moment.

I get the point about dipoles no longer being a special case, so the default option exercises the new approach automatically. I'll try to think if there's something else we can do to exercise this.

I think committing this soon in its current form makes sense. It consistently and nicely extends the present behavior of SampledCorrelations to LSWT. Maybe we just accept some doc duplication for SpinWaveTheory at the moment. We can have a discussion before making any more substantial interface commitments.

[kbarros]

I suppose parse_observables could just be changed into the constructor for ObservableInfo

I like this idea -- in our eventual final interface, the user can construct their own ObservableInfo and then pass it pre-formed to the various intensity formula functions. But given the current state of flux, we can hold off on that change for now.

Here is another question that is more urgent: Am I understanding correctly that with :full the intensities_bands function now returns a list of SMatrix{3,3} rather than a flattened Vector of 9 elements? This seems to be the origin of the breakage in test_lswt.jl, and will also break user-code that happens to be using :full (did we include anything like this in our examples?). Is this a breaking change we are motivated to make, and do we want to make it now? What are the tradeoffs?

• If we want to make this breaking change, then we at least need to add documentation in versions.md.
• I am thinking maybe we don't want to make this breaking change. My long term goal is to remove all dynamic typing behavior from the intensities* functions (except maybe some very simple conversions in the constructors). It's a simple story to say: The 3x3 matrix of intensities should be "just another list of observable pairs".

[kbarros]

Regarding the reshaping mystery, the mistake was forgetting to include the final fourth argument (NObs2) when defining Contraction{SMatrix{NObs, NObs, ComplexF64, NObs2}}. With the latest commit, https://github.com/SunnySuite/Sunny.jl/pull/167/commits/d65c8020baec5fdf6ba9e1879d5c2f9929a1e8d8, it is fixed. Now the reshapings in test_lswt.jl work again.

The final question to address: does this introduce a breaking change, and do we want to do that? Need input from both @Lazersmoke and @ddahlbom, maybe on a short call.

[Lazersmoke]

The previous behavior was to dump whatever correlations were stored in sc.data on the user with little explanation. They ended up getting a six element vector from SampledCorrelations, or a 9 element vector for SWT, neither of which really has a canonical ordering, especially in the presence of general observables. I recently sent someone this line of code from deep within sunny to explain what they were getting from :full (in v0.4.3):

https://github.com/SunnySuite/Sunny.jl/blob/370d8016c3ac6fcd8387e59b6b02a9a0583eaf8b/src/StructureFactors/ElementContraction.jl#L46

The correlation matrix (3x3 by default) is what users really want to interact with when they ask for the full tensor

[ddahlbom]

I agree with Sam that the previous behavior was close to uninterpretable, and putting it into 3x3 matrix is the most common thing a person would want from a :full call. But I also agree that it's sort of a special case that doesn't sit well with the current set of abstractions: every other "mode" will give a number, and in principle :full should return some kind of sparse matrix, the size and interpretation of which depends on the observable information given.

The :full approach is also conceptually more fundamental, i.e. it is what exists before a contraction and should correspond in some sense to a "no op" contraction.

I think we may want to move away from the current Contractor approach altogether. Conceptually (not necessarily from an interface standpoint), there should be a layer of functions that return the "full" results (in obscure form). There should probably be functions that take these results and the observable info and produce some correlation matrix that's user friendly. And then there should be contraction functions (probably not as a type type).

The trickiness lies in how to resolve this in a way that is both performant and presents a reasonable user interface.

Many decisions for the original SampledCorrelations intensities code were based on the notion that it might be plugged into an interactive plotting interface, so a lot of effort was made to pool many steps together without allocations. I'm not sure this is really a priority any more. We can probably do something that has multiple clearly-separated stages of processing and accept allocations between these stages.