Closed Hao-Phys closed 1 year ago
Hi Hao, thanks for the PR. Overall this looks great to me. My suggestions are mostly concerning the public-facing user interface. Maybe we should discuss together with @ddahlbom . The implementation details can always be revised later, so I want to focus for now one the question of what exported functions we should be exposing.
Are we good to merge now with some minor renamings? We can rename more later in a follow up.
What are the renamings we need to do right now?
I'm just thinking about making the API as similar as possible to what its final form will be and making it so that the FeI2 example will look as clean as possible in the meantime. In this spirit, and based on the discussion above, I would suggest:
1) lswt_unpolarized_INS_spec
-> intensities
.
2) lswt_dispersion_relation
-> dispersion
.
3) lswt_dynamical_spin_structure_factor!
-> dssf
. It would also be good if the Sαβ_matrix
could be allocated internally. (Another possible name for this function would be dynamic_structure_factor
, but I think it's a bit odd that we would briefly have both dynamic_structure_factor
and DynamicStructureFactor
.)
4) SpinWaveFields
-> SpinWave
The naming/interface for (3) and (4) will probably be subject to the most changes later on, but I think these changes would be good for now.
These suggestions look good to me. Thanks all, let's get it merged 👍
Kipton Barros, from phone
On Sun, Mar 19, 2023, 12:52 David A. Dahlbom @.***> wrote:
I'm just thinking about making the API as similar as possible to what it's final form will be and making it so that the FeI2 example will look as clean as possible. In this spirit, and based on the discussion above, I would suggest:
- lswt_unpolarized_INS_spec -> intensities.
- lswt_dispersion_relation -> dispersion.
- lswt_dynamical_spin_structure_factor! -> dssf. It would also be good if the Sαβ_matrix could be allocated internally. (Another possible name for this function would be dynamic_structure_factor, but I think it's a bit odd that we would briefly have both dynamic_structure_factor and DynamicStructureFactor.)
- SpinWaveFields -> SpinWave
The naming/interface for (3) and (4) will probably be subject to the most changes later on, but I think those changes would be good for now.
— Reply to this email directly, view it on GitHub https://github.com/SunnySuite/Sunny.jl/pull/68#issuecomment-1475319227, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACG3HM325ZFEKNIGS5AHSDW442VLANCNFSM6AAAAAAV57FKLQ . You are receiving this because you commented.Message ID: <SunnySuite/Sunny .@.***>
I have changed the function names based on David's suggestions. Moreover, I have added supports for biquad
in the :dipole
mode (and a corresponding test item in test_lswt.jl). We are ready to merge.
This PR adds the SU(N) linear spin-wave theory features to Sunny. The
:SUN
and: dipole
modes are supported. But the:large_S
mode is not supported yet. Moreover, thebiquad
interactions are not supported.