-
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
Merge prototype version 0.5.0 staging branch into main #90
Conversation
* Add `intensities_binned` * Refactored interface for intensity calculations * Implemented `:dipole` mode for LSWT * Add `load_nxs_binning_parameters` * Refactor file structure * Merge SWT dipole calcs with intensity_formula --------- Co-authored-by: ddahlbom <david.dahlbom@gmail.com> Co-authored-by: Hao Zhang <hzhangphys@gmail.com>
Vectorization is just good as it turns out
Renamed from one_dimensional_cut
Thanks Xiaojian! This is the best we can do with the available fields of the `.nxs`
# SQTODO: Make this use the more optimized expected_spin function | ||
# Doing this will also, by necessity, allow users to make the same | ||
# type of optimization for their vector-valued observables. | ||
observables = LinearMap{ComplexF64}.(spin_matrices(N)) |
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.
What is the motivation for using LinearMap
instead of a dense Hermitian{ComplexF64}
? If all else is equal, I'd prefer the explicit option. One could imagine that LinearMap
could be useful for optimization if we have a very fast matrix-vector product available, but will that be the case in our intended applications?
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 main reason is that this (should) compile to the same thing as it did before this change specifically for dipole mode. A linear map s -> s[i]
is better than multiplying by a row vector IMO. It's also nicer for advanced users to specify a linear map, especially in sun mode
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.
Do you have examples in mind of linear maps that are not explicit matrices?
Nevermind I understand your example now.
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.
BTW, one place this might show up is in LSWT where we can in principle construct unitaries exp(i theta S)
as a polynomial of sparse spin operators S. But maybe that type of thing never appears for observables.
JSON = "682c06a0-de6a-54ab-a142-c8b1cf79cde6" | ||
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" | ||
LinearMaps = "7a12625a-238d-50fd-b39a-03d52299707e" |
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.
Every additional package will affect load time. It seems like JLD2 will bring a lot of value, but maybe we can work around LinearMaps?
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.
LinearMaps seems relatively lightweight:
https://juliahub.com/ui/Packages/LinearMaps/bdfwy/3.10.2?page=1
If you think it will make the interface more convenient then I'm OK with including this package.
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.
Yep, it's lightweight and in some sense the "correct" way to do certain kinds of linear algebra in Julia. We're bound to need it at some point
src/Sunny.jl
Outdated
add_sample!, broaden_energy, lorentzian, | ||
all_exact_wave_vectors, ωs, spherical_shell, merge!, intensity_formula, integrated_lorentzian | ||
|
||
include("Intensities/ElementContraction.jl") | ||
|
||
include("Intensities/Interpolation.jl") | ||
export intensities_interpolated, instant_intensities_interpolated, connected_path | ||
|
||
include("Intensities/Binning.jl") | ||
export intensities_binned, instant_intensities_interpolated, connected_path_bins, | ||
BinningParameters, integrate_axes!, unit_resolution_binning_parameters, | ||
rlu_to_absolute_units!, | ||
intensities_binned, slice_2D_binning_parameters, axes_bincenters, | ||
connected_path_bins, count_bins | ||
|
||
|
||
include("Intensities/PowderAveraging.jl") | ||
export powder_averaged_bins, powder_average | ||
|
||
include("Intensities/ExperimentData.jl") | ||
export load_nxs, generate_mantid_script_from_binning_parameters |
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 is a long list of exports. Have all of them been carefully considered, and extensive doc strings written? (I'm not familiar with these functions in particular, so please take this as a general comment).
Context: The export list is part of the package's SemVer guarantee. That is, there is an implicit promise not to change the behavior of functions if they are exported. If any of these functions may still be evolving, we can hide them from the export list, while access is still possible through the syntax Sunny.my_unexported_function
.
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.
Two specific cases are mentioned above. Namely, do we want to export rlu_to_absolute_units!
? And should we export powder_average
(and therefore add a docstring)?
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.
Context: The export list is part of the package's SemVer guarantee. That is, there is an implicit promise not to change the behavior of functions if they are exported. If any of these functions may still be evolving, we can hide them from the export list, while access is still possible through the syntax
Sunny.my_unexported_function
.
I've just gone through and cleaned up the exports a bit. In terms of SemVer guarantee, we need to settle the units of the BinningParameters
before unleashing this on the world, but once it is settled, BinningParameters
, count_bins
, axes_bincenters
, rlu_to_absolute_units!
(or its replacement) area all part of the specification of what the parameters mean.
If e.g. additional method definitions were added to slide_2D_binning_parameters
, that would be ok SemVer-wise, right?
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.
@ddahlbom I think that's right re: powder_average_interpolated
, since that function can (and is) implemented in terms of intensities_interpolated
, whereas powder_average_binned
is impossible to implement correctly in terms of intensities_binned
. I vote un-export powder_average_interpolated
but mention its existence in the Example
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.
I'm fine with not exporting it. (Reference to it should then be eliminated from the doc string for the binned version.)
Great work Sam! I put some comments above for you to consider, but David's review will be more important. |
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.
Thanks for putting this all together. It looks great to me. I added a few minor comments in the code.
function Base.show(io::IO, ::MIME"text/plain", sf::StructureFactor) | ||
modename = sf.dipole_corrs ? "Dipole correlations" : "Custom correlations" | ||
printstyled(io, "StructureFactor [$modename]\n"; bold=true, color=:underline) | ||
function Base.show(io::IO, sf::StructureFactor{N}) where N |
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.
I wonder if we should reconsider the name StructureFactor
, since we're now adding a number of other ways to get S(q,w) information. I guess it could just be ClassicalStructureFactor
, though I dislike how long it is. Any thoughts?
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.
A more appropriate name might be LandauLifschitzCorrelations
, or a name appropriately generalizing this which includes both dipole and SU(N) cases. Perhaps Correlations{N}
with a type synonym DipoleCorrelations = Correlations{0}
?
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.
I like this idea for the reason that I was initially confused by the fact that no part of using spin wave theory involves constructing a StructureFactor
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.
Agree it's confusing, though the basic reason is an asymmetry in the calculations; there's never a need to explicitly store structure factor data as an intermediate step with case of SWT, whereas for the classical calculations you have a chunk of data that is literally all of S(q,w) and some metadata.
I think some form of Correlations
is ok, but it's technically stored as the Fourier transform of the spin correlations (i.e., it is the structure factor for a finite-sized system). It might be confusing to a user expecting to get real space correlations directly. But I'm not completely opposed to it.
I tend to favor using Classical
to cover both dipole and SU(N) classical dynamics (it avoids having to make a distinction between traditional LL and its generalization).
I think the type synonym might be confusing, since many users will just look at dipole information even though they're in SU(N) mode.
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.
Yep ok agreed. I think this is a good idea in general, but out of scope for the PR :)
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.
Good point. I'm comfortable with merging if you are! (I guess once you've done what you like with the binning units.)
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.
Yep absolutely! I'm nearly done converting everything to use absolute units everywhere, it's quite thorny to track down every place where RLU has been assumed.
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.
Sounds good. Just to be clear, when you say "everywhere," do you mean in the binning functions? I wouldn't change the default behavior of the SWT or classical interfaces. RLUs make a lot of sense as the default units elsewhere, I think. (For example, it's much more meaningful to say that the ordering wave vector is (pi, pi, pi) than to give the same information in absolute units.)
src/Sunny.jl
Outdated
add_sample!, broaden_energy, lorentzian, | ||
all_exact_wave_vectors, ωs, spherical_shell, merge!, intensity_formula, integrated_lorentzian | ||
|
||
include("Intensities/ElementContraction.jl") | ||
|
||
include("Intensities/Interpolation.jl") | ||
export intensities_interpolated, instant_intensities_interpolated, connected_path | ||
|
||
include("Intensities/Binning.jl") | ||
export intensities_binned, instant_intensities_interpolated, connected_path_bins, | ||
BinningParameters, integrate_axes!, unit_resolution_binning_parameters, | ||
rlu_to_absolute_units!, | ||
intensities_binned, slice_2D_binning_parameters, axes_bincenters, | ||
connected_path_bins, count_bins | ||
|
||
|
||
include("Intensities/PowderAveraging.jl") | ||
export powder_averaged_bins, powder_average | ||
|
||
include("Intensities/ExperimentData.jl") | ||
export load_nxs, generate_mantid_script_from_binning_parameters |
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.
Two specific cases are mentioned above. Namely, do we want to export rlu_to_absolute_units!
? And should we export powder_average
(and therefore add a docstring)?
|
||
function intensity_formula_kpm(f::Function,swt::SpinWaveTheory,corr_ix::AbstractVector{Int64}; P =, return_type = Float64, string_formula = "f(Q,ω,S{α,β}[ix_q,ix_ω])") | ||
|
||
error("KPM not yet implemented") |
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 plan is this gets clobbered in the merge with kpm branch eventually
if !isnothing(sys.origin) | ||
sys_origin = resize_periodically(sys,sys.origin.latsize) | ||
|
||
# Translate to a correct lattice site in the middle of the bigger system | ||
center_origin = (sys_origin.crystal.latvecs * Vec3(sys_origin.latsize))/2 | ||
center_this = (sys.crystal.latvecs * Vec3(sys.latsize))/2 | ||
center_offset = center_origin .- center_this | ||
middle_site = global_position(sys_origin,CartesianIndex((sys_origin.latsize .÷ 2)...,1)) | ||
diff_center = center_offset .- middle_site | ||
|
||
pts = Makie.Point3f0.(map(x -> diff_center .+ x,spin_vector_origins(sys_origin, arrowlength))[:]) | ||
vecs = Makie.Vec3f0.(sys_origin.dipoles[:]) | ||
Makie.arrows!( | ||
ax, pts, vecs; | ||
linewidth, arrowsize, | ||
lengthscale=arrowlength, | ||
# Original system is transparent | ||
arrowcolor=(arrowcolor,0.03), linecolor = (linecolor,0.03), | ||
kwargs... | ||
) | ||
end |
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 may have been better as a separate PR, but it does this:
which I think is neat and resolves the confusion I had looking at this example for the longest time (I thought "supercell" meant that each of the four spins was meant to represent a cluster of spins in the original system, rather than a sublattice)
Assuming the CI goes ok, I'll rebase/squash & merge this later today, hopefully in a sensible way |
This is to collect and resolve all the merge conflicts that are about to result from the various great changes everyone is working on :)
Changes incorporated so far are listed below:
Implemented
:dipole
mode for LSWT via @Hao-Phys closes Swt dipole #88Add
intensities_binned
Refactored interface for intensity calculations
Add
load_nxs_binning_parameters
Refactor file structure
Merge SWT dipole calcs with intensity_formula