Lswt general observables #156

Lazersmoke · 2023-09-04T04:51:34Z

This adds support for arbitrary user-specified observables (NxN matrix) to LSWT SU(N) mode by factoring out the existing custom-observable support from SampledCorrelations into a new struct ObservableInfo, which then is shared between LSWT and classical modes. Note that LSWT dipole mode does not support NxN matrix observables; it only supports the default Sx,Sy,Sz observables.

In addition, this change considerably simplifies SampledCorrelations because it no longer needs to worry about the observables as much

Also elaborates the user-facing `show` instance for `System` to describe more features (external field, long range dipole-dipole mode, summarize interactions, more informative reshaping info)

Co-authored-by: Sam Quinn <[email protected]>

(Thanks Hao!)

kbarros · 2023-09-04T18:45:52Z

This PR builds on top of #125, so that one would need to be merged first.

It looks like the new thing here is the "Add ObservableInfo" commit.

Separately, the "Upgrade developer-facing show instances" commit no longer exists on main, and now lives separate branch, detailed-show. We can revisit which of those features to merge into main at a later time.

Lazersmoke · 2023-09-04T19:18:19Z

Yep, for some reason the merge is much messier when targeting #125 directly.

Edit: and the reason for building off of #125 is that it was easier to build off the existing LSWT refactor there.

Lazersmoke · 2023-09-04T19:42:09Z

test/test_binning.jl

@@ -94,7 +94,7 @@
      is_flat[m + (k-1) * 6,1,1,l] = is[k,1,1,l][m]
    end

-    @test isapprox(is_flat,is_golden;atol = 1e-12)
+    @test_broken isapprox(is_flat,is_golden;atol = 1e-12)


Note that I have disabled these tests as "broken" -- something strange is going on because the classical :full appears broken while the LSWT :full is working.

I think you just need to update the test. After your refactor, is[k,1,1,l][m] is indexing into a 3x3 array rather than a length-6 SVector.

The following quick hack fixes the test.

is_flat = zeros(ComplexF64,size(is_golden)) midcs = collect(keys(sc.observables.correlations)) |> reverse for k = 1:4, l = 1:3, m = 1:6 is_flat[m + (k-1) * 6,1,1,l] = is[k,1,1,l][midcs[m]] end isapprox(is_flat,is_golden;atol = 1e-12) # Now passes

ddahlbom · 2023-09-04T19:48:33Z

I'll take a look a little later today.

…

On Mon, Sep 4, 2023, 15:42 Sam Quinn ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/test_binning.jl <#156 (comment)>: > @@ -94,7 +94,7 @@ is_flat[m + (k-1) * 6,1,1,l] = is[k,1,1,l][m] end - @test isapprox(is_flat,is_golden;atol = 1e-12) + @test_broken isapprox(is_flat,is_golden;atol = 1e-12) Note that I have disabled these tests as "broken" -- something strange is going on because the classical :full appears broken while the LSWT :full is working. — Reply to this email directly, view it on GitHub <#156 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIERZRZJXYZGI77O6DYUSATXYYVJXANCNFSM6AAAAAA4J2Y5UA> . You are receiving this because your review was requested.Message ID: ***@***.***>

Lazersmoke · 2023-09-04T22:29:55Z

Thanks David, good catch! Glad it is just the test that's broken 😅, will update it soon

…

On Mon, Sep 4, 2023, 18:28 David A. Dahlbom ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/test_binning.jl <#156 (comment)>: > @@ -94,7 +94,7 @@ is_flat[m + (k-1) * 6,1,1,l] = is[k,1,1,l][m] end - @test isapprox(is_flat,is_golden;atol = 1e-12) + @test_broken isapprox(is_flat,is_golden;atol = 1e-12) I think you just need to update the test. After your refactor, is[k,1,1,l][m] is indexing into a 3x3 array rather than a length-6 SVector. The following quick hack fixes the test. is_flat = zeros(ComplexF64,size(is_golden)) midcs = collect(keys(sc.observables.correlations)) |> reverse for k = 1:4, l = 1:3, m = 1:6 is_flat[m + (k-1) * 6,1,1,l] = is[k,1,1,l][midcs[m]] end isapprox(is_flat,is_golden;atol = 1e-12) # Now passes — Reply to this email directly, view it on GitHub <#156 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAW3RJAZQ6XMEC6RRV5JC5LXYZIX3ANCNFSM6AAAAAA4J2Y5UA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ddahlbom

I left some comments and questions in the code. These are limited primarily to the new Observables infrastructure. It would be good to break this material out into an isolated PR, if possible.

I think pulling out the observable functionality and reusing it between SWT and SampledCorrelations is a great approach. That all looks good to me.

I'd hold off on expanding the SWT code to handle general observables for the time being, since we're probably going to do some significant refactors to SWT code in the near future. It would be good to leave that code as uncomplicated as possible in the meanwhile. So, for example, I wouldn't include the general SU(N) basis.

But, overall, this sets the right foundation for general observables.

ddahlbom · 2023-09-05T00:15:08Z

src/Operators/Observables.jl

+    # Correlation info (αβ indices of S^{αβ}(q,ω))
+    observables    :: Vector{LinearMap}  # Operators corresponding to observables
+    observable_ixs :: Dict{Symbol,Int64} # User-defined observable names
+    correlations   :: SortedDict{CartesianIndex{2}, Int64}  # (α, β) to save from S^{αβ}(q, ω)


One thing I realized recently is that the correlations seem to be sorted in an order opposite of what I would expect. In the case of SampledCorrelations, it will create loops that go (in the default case) from 6 down to 1.

CartesianIndex(1, 1) => 6 CartesianIndex(1, 2) => 5 CartesianIndex(2, 2) => 4 CartesianIndex(1, 3) => 3 CartesianIndex(2, 3) => 2 CartesianIndex(3, 3) => 1

In other words, the indices are sorted in terms of the abstract ordering of the named correlations, but this is reverse with respect to memory access.

Is there some fundamental reason for this? (My memory is a bit foggy on the details of what is constraining these decisions.)

The SortedDict is apparently for performance (there was some comment to this effect in the SampledCorrelations constructor). The common use case is that you want to visit each correlation, ask what two variables are correlated, and then also know which index to save that to (this is accomplished by simple iteration over the Dict). Also, a Dict has the advantage (over Vector{CartesianIndex{2}}) of disallowing saving the same correlation twice.

I don't think there is any major performance difference regarding memory access order here

Yeah, I agree that I don't think it will have serious performance implications.

ddahlbom · 2023-09-05T00:15:30Z

src/Operators/Observables.jl

@@ -0,0 +1,141 @@
+
+struct ObservableInfo


Breaking this out is a good approach, I think.

ddahlbom · 2023-09-05T00:35:14Z

src/SampledCorrelations/SampledCorrelations.jl

-    samplebuf = zeros(ComplexF64, length(observables), sys.latsize..., na, nω) 
-    data = zeros(ComplexF64, ncorr, na, na, sys.latsize..., nω)
+    samplebuf = zeros(ComplexF64, num_observables(observables), sys.latsize..., na, nω) 
+    data = zeros(ComplexF64, num_correlations(observables), na, na, sys.latsize..., nω)


These num_observables, num_correlations functions are nice.

ddahlbom · 2023-09-05T00:37:01Z

src/Operators/Observables.jl

+end
+
+
+function parse_observables(N;observables = nothing,correlations = nothing)


It would be nice to add a quick (internal) comment explaining what this function does so one can figure it out quickly. Seems like prepare_observables might be a slightly more appropriate name (since, for instance, it will also set up default observables if none are given).

Sounds good. It is parsing several possible formats of user-supplied observable operators and formats of correlations

ddahlbom · 2023-09-05T00:48:22Z

src/SpinWaveTheory/SpinWaveTheory.jl

+    dipole_operators :: Array{ComplexF64, 4}
+    onsite_operator :: Array{ComplexF64, 3}  # Single-ion anisotropy
+    quadrupole_operators :: Array{ComplexF64, 4}
+    sun_basis :: Array{ComplexF64, 4}


I think we probably should not include an entire explicit basis inside the SWT object. Part of the problem is that the size of the basis for the operator algebra grows quadratically in N, so this quickly becomes nontrivial for large N. We have also been trying to stay away from the specification of explicit bases.

I think we probably will want to go to a situation where the user specifies an arbitrary set of specific observables when creating the SWT object, and these will be the observables that are transformed and toted around in the SWT calculation. This is of course less general, but it will keep things consistent with the SampledCorrelations infrastructure. It's pretty cheap to recreate an SWT object with whatever observables you like, so one doesn't save too much by including a general basis.

Kipton and I have been discussing some refactors of this that will be put into place once the "entangled units" infrastructure is in place for the classical stuff, at which point we will likely only keep two arrays in here (pairs of bond-associated operators and then a list of observables). So this type will probably go under some major revisions once we're through the workshop.

All of this is to say, it's probably best to keep the code simpler and hold off on including this basis for now. (It doesn't look like you make use of it yet in any case.)

Yes, it can be removed.

Re: the other refactor, the custom observable code is actually much less complicated compared to the previous code, which was effectively using "magic numbers" (1,2,3) to specify the spin observables instead of accepting arbitrary observable matrices and using those. I recommend the new code (here and in the lswt_speedup) as a base for any future refactors

Yeah, completely agree what you have is a big improvement. Regarding complications, I was only referring to leaving out the SU(N) basis for now.

ddahlbom · 2023-09-05T00:57:32Z

src/SpinWaveTheory/SWTCalculations.jl

+                    (α,β) = ci.I
+                    corrs[i] = Avec[α] * conj(Avec[β])
+                end
+                corrs


This looks like a nice solution for the moment, i.e., it will generalize but is currently restricted to dipoles.

ddahlbom · 2023-09-05T14:46:33Z

Looked at this again a bit more closely this morning. There are a few things that didn't sink when I was going through the diff yesterday.

This all looks exactly like what we want and should not interfere with future refactors (since, for example, the observables in the SWT at this point keep their own rotated spin operators -- this will allow us to remove the spin and quadrupole operators from the SWT struct without issue).

ddahlbom · 2023-09-05T14:50:45Z

src/SpinWaveTheory/SWTCalculations.jl

@@ -634,46 +713,50 @@ function intensity_formula(f::Function,swt::SpinWaveTheory,corr_ix::AbstractVect
            phase = exp(-2π*im * dot(q_reshaped, sys.crystal.positions[i]))
            Avec_pref[i] = sqrt_Nm_inv * phase

-             # TODO: move form factor into `f`, then delete this rescaling
+            # TODO: move form factor into `f`, then delete this rescaling


What would be the approach to doing this? Basically pull the loop below (Fill 'intensity' array) inside of f one way or another?

Lazersmoke · 2023-09-05T16:09:41Z

Hmm so I think we still need the dipole and quadrupole operators in the SWT struct for constructing the Hamiltonian. Unless the future refactor constructs it without needing those? It's true that they are not needed for the reading out of values at the end

…

On Tue, Sep 5, 2023, 10:46 David A. Dahlbom ***@***.***> wrote: Looked at this again a bit more closely this morning. There are a few things that didn't sink when I was going through the diff yesterday. This all looks exactly like what we want and should not interfere with future refactors (since, for example, the observables in the SWT at this point keep their own rotated spin operators -- this will allow us to remove the spin and quadrupole operators from the SWT struct without issue). — Reply to this email directly, view it on GitHub <#156 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAW3RJF2GYJGKXQXWBBTDXTXY43NJANCNFSM6AAAAAA4J2Y5UA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ddahlbom · 2023-09-05T16:36:27Z

Right. The interactions in the Hamiltonian will not be constructed in terms of relations between spin operators, quadrupoles, etc., but instead in terms of a set of bond-dependent operators that render the interactions maximally sparse. Kipton worked it out, wrote a Lyx note about it. I think it's in the repo. Familiar observables will only appear when initially specifying interactions and when querying intensities.

Lazersmoke · 2023-09-07T19:19:30Z

OK this is almost ready except for one detail: The OnsiteCoupling has changed to a union type at some point, so this needs to be made consistent. This branch also includes detailed-show, which also needs to be updated (it currently prints both the matrix and the stevens expansion, which doesn't make sense with the union type)

Side note about this: The stevens expansion is always traceless!!! So it seems to me like a bad idea to ever just use only the stevens expansion

kbarros · 2023-09-07T21:02:21Z

This branch also includes detailed-show, which also needs to be updated (it currently prints both the matrix and the stevens expansion, which doesn't make sense with the union type)

Could you please create a commit that lives on top of main and just has the Observables updates? (it's not clear what will happen to detailed-show, let's not stack PRs.)

The stevens expansion is always traceless!!! So it seems to me like a bad idea to ever just use only the stevens expansion

The matrix representation is also shifted to be traceless. The current design disallows an onsite coupling to make a constant shift to the energy.

kbarros · 2023-09-09T15:32:53Z

The stevens expansion is always traceless!!! So it seems to me like a bad idea to ever just use only the stevens expansion

The matrix representation is also shifted to be traceless. The current design disallows an onsite coupling to make a constant shift to the energy.

This is fixed in db7eab5. Now the matrix trace is retained, which corresponds to keeping the $\mathcal{O}_{0,0}$ term of the Stevens expansion.

This change allows anisotropy operators to introduce a constant shift in the energy, but otherwise, shouldn't have any effect on physical observables. In SU(N) mode we never cared about this constant energy shift before, but now that the user can set "large S" anisotropies, the change becomes important for reducing confusion.

And add regression test for next time this breaks

kbarros · 2023-09-19T01:34:05Z

Closing in favor of #167 which squashes the commits and rebases them on main.

Lazersmoke and others added 10 commits September 3, 2023 16:53

Upgrade developer-facing show instances

e165bef

Also elaborates the user-facing `show` instance for `System` to describe more features (external field, long range dipole-dipole mode, summarize interactions, more informative reshaping info)

Display sys in examples

d8b0881

Optimizations to LSWT

d7d57d7

Co-authored-by: Sam Quinn <[email protected]>

Refactor checkpoint (broken)

78e5a35

Strange Conjugation?

358c39d

Properly take care of H22

c380caa

(Thanks Hao!)

add test

93a42b1

lswt langasite test.

29d6b8b

Update test_lswt.jl

07d9582

Add ObservableInfo

cde9dfb

kbarros requested review from ddahlbom and kbarros September 4, 2023 13:14

kbarros force-pushed the main branch from d8b0881 to 5aea539 Compare September 4, 2023 15:02

Lazersmoke changed the base branch from main to lswt_speedup September 4, 2023 19:16

Lazersmoke changed the base branch from lswt_speedup to main September 4, 2023 19:17

Lazersmoke commented Sep 4, 2023

View reviewed changes

Fix tests to use 3x3 correlation matrix

b7be529

ddahlbom reviewed Sep 5, 2023

View reviewed changes

Merge branch 'main' into lswt_general_observables

5af0cbe

Lazersmoke added 3 commits September 18, 2023 15:00

Fix dipole factor

1df7b1a

And add regression test for next time this breaks

Revert detailed-show

401830d

Fix test

e21868d

kbarros mentioned this pull request Sep 19, 2023

General Observables 2 #167

Merged

kbarros closed this Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lswt general observables #156

Lswt general observables #156

Lazersmoke commented Sep 4, 2023 •

edited

Loading

kbarros commented Sep 4, 2023 •

edited

Loading

Lazersmoke commented Sep 4, 2023 •

edited

Loading

Lazersmoke Sep 4, 2023

ddahlbom Sep 4, 2023 •

edited

Loading

ddahlbom commented Sep 4, 2023 via email

Lazersmoke commented Sep 4, 2023 via email

ddahlbom left a comment

ddahlbom Sep 5, 2023

Lazersmoke Sep 5, 2023

ddahlbom Sep 5, 2023

ddahlbom Sep 5, 2023

ddahlbom Sep 5, 2023

ddahlbom Sep 5, 2023

Lazersmoke Sep 5, 2023

ddahlbom Sep 5, 2023

Lazersmoke Sep 5, 2023

Lazersmoke Sep 5, 2023

ddahlbom Sep 5, 2023

ddahlbom Sep 5, 2023

ddahlbom commented Sep 5, 2023

ddahlbom Sep 5, 2023 •

edited

Loading

Lazersmoke commented Sep 5, 2023 via email •

edited

Loading

ddahlbom commented Sep 5, 2023 •

edited

Loading

Lazersmoke commented Sep 7, 2023

kbarros commented Sep 7, 2023

kbarros commented Sep 9, 2023

kbarros commented Sep 19, 2023

		end


		function parse_observables(N;observables = nothing,correlations = nothing)

Lswt general observables #156

Lswt general observables #156

Conversation

Lazersmoke commented Sep 4, 2023 • edited Loading

kbarros commented Sep 4, 2023 • edited Loading

Lazersmoke commented Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

ddahlbom Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

ddahlbom commented Sep 4, 2023 via email

Lazersmoke commented Sep 4, 2023 via email

ddahlbom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddahlbom commented Sep 5, 2023

ddahlbom Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Lazersmoke commented Sep 5, 2023 via email • edited Loading

ddahlbom commented Sep 5, 2023 • edited Loading

Lazersmoke commented Sep 7, 2023

kbarros commented Sep 7, 2023

kbarros commented Sep 9, 2023

kbarros commented Sep 19, 2023

Lazersmoke commented Sep 4, 2023 •

edited

Loading

kbarros commented Sep 4, 2023 •

edited

Loading

Lazersmoke commented Sep 4, 2023 •

edited

Loading

ddahlbom Sep 4, 2023 •

edited

Loading

ddahlbom Sep 5, 2023 •

edited

Loading

Lazersmoke commented Sep 5, 2023 via email •

edited

Loading

ddahlbom commented Sep 5, 2023 •

edited

Loading