SunnySuite / Sunny.jl

Spin dynamics and generalization to SU(N) coherent states
Other
86 stars 19 forks source link

Optimizations to LSWT #125

Closed kbarros closed 1 year ago

kbarros commented 1 year ago

Sam's LSWT optimizations.

kbarros commented 1 year ago

Is it possible to make a complicated (and arbitrary) LSWT model that exercises everything in a simple unit test at some arbitrary wavevector? This might be possible without much work if the wavevector is commensurate. Having that in place seems nicer prior to optimizations.

Lazersmoke commented 1 year ago

I will say, as I'm making more changes and actually breaking things accidentally, the existing test_lswt appears to do a good job of catching the errors

kbarros commented 1 year ago

Yeah the current tests are super helpful.

Specific things that would be nice to additionally test:

I think @hlane33 may be working on this?

hlane33 commented 1 year ago

This is correct @kbarros.

Hao-Phys commented 1 year ago

Hi @Lazersmoke, thanks a lot for making lswt fast! Your changes look great to me. Glad to know that the current test_lswt is able to catch errors. For the more general example, I suggest that we should ask the experimentalists for the most complicated models they have in mind. Then we use spinW to check.

Hao-Phys commented 1 year ago

Actually, if I recall correctly. FeI2's s(q,w) does have off-diagonal components. This model is quite complicated. We can use spinW to compute the SU(2) spectra and compare. (Potentially we can also add artificial DM interactions if they are symmetry-allowed)

kbarros commented 1 year ago

Agreed that it's nice to reproduce various experimentally relevant calculations.

But for purposes of a unit test, an artificial model filled with the "kitchen sink" of features might serve best:

We can calculate the (:SUN and :dipole) intensities once, and then right a unit test to make sure these results never change as we update the LSWT code. We can also compare with the SpinW result in dipole mode (for this, it's possible we need to turn off biquadratic in SpinW if we don't trust the SpinW result).

Lazersmoke commented 1 year ago

Since this is blocked on testing anyway, I went ahead and made some more renamings, simplifications, etc. Something appears to be subtly broken now, but I've tracked it down to having to do with the relation between specifically Hmat11 and Hmat22. Are these not supposed to be complex conjugate transposes of each other? It seems that they are transposed, but only the phase exp(iq . Delta R) is conjugated. Ideally we would like to compute only Hmat11, and then conjugate to get Hmat22. Maybe @Hao-Phys can comment if this is possible?

Hao-Phys commented 1 year ago

Hi @Lazersmoke . No the relation is not simply a transpose. The relation is give by Hmat22(k)=transpose(Hmat11(-k))

Lazersmoke commented 1 year ago

It now passes tests and should be correct 🎉 ! Thank you!

Lazersmoke commented 1 year ago

Before optimization:

image

After:

image

Lazersmoke commented 1 year ago

I've just rebased this so that we're up-to-date on the Requires.jl change

kbarros commented 1 year ago

This is still blocked on a "golden reference" test. Per our discussion yesterday, the test doesn't need to be very physical, and we don't need an analytical result. The test just needs to verify that the behavior of the LSWT calculation will not change for any conceivable input. It sounded like @hlane33 plans to implement such a test, is that correct? Once we get that I am happy to merge this.

hlane33 commented 1 year ago

Yes I will put something together by the end of the weekend

Lazersmoke commented 1 year ago

It's actually quite difficult to put everything at once into once test, but I've put everything you could imagine for an unphysical SU(N) test into a kitchen sink test, and verified that it also passes on main. Whether there is multi-Q ordering is left as an exercise to the reader

Update: This passes locally! And only the dispersion test fails on CI (all of the intensity stuff [which should all be harder, surely??] passes locally and on CI)

Update update: This passes when I copy paste the test into the julia terminal, but then it fails using ]test Sunny

kbarros commented 1 year ago

We merged Sam's nice SU(N) "golden test" here: https://github.com/SunnySuite/Sunny.jl/commit/c8aec95061218229bb1b4db8d777d1a0e479c13b

The deviation between "terminal" and "test" environments @Lazersmoke mentioned above could be due to Julia configuration (@Lazersmoke tracked it down to the three-term dot function). Since the difference is only in the last digit, a solution is to weaken the test tolerance.

I cleaned up the Langasite test, and @hlane33 is welcome to review the new version.

I rebased on main as two commits, and I think we can merge them without squashing.