Skip to content
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

Wavevectors should be specified in absolute units. #116

Merged
merged 13 commits into from
Aug 7, 2023
Merged

Conversation

kbarros
Copy link
Member

@kbarros kbarros commented Aug 4, 2023

This is a work in progress.

Provide cryst.recipvecs in analogy to cryst.latvecs. Using this, it becomes easy to manually convert from RLU to absolute units before calling Sunny functions.

The FeI2 tutorial has been updated. Averaging of 120 degree rotations becomes natural in absolute units.

Significant simplifications to LSWT become possible by working in absolute units everywhere. In particular, it is no longer necessary to keep track of an "original" crystal.

The tests involving SampledCorrelations and binning are failing. This part of the code needs to be updated to use absolute units. I think this will lead to nice simplifications. See some "FIXME" markers for a subset of things that should change. Functions like bin_absolute_units_as_rlu! and bin_rlu_as_absolute_units! are no longer needed.

@ddahlbom
Copy link
Member

ddahlbom commented Aug 6, 2023

I realized this has very little impact on most of the sampled correlations material. I basically added one line of code at the beginning of intensities_interpolated to convert to the input to RLU with respect to the reshaped crystal, and I changed all_exact_wave_vectors so that it returns absolute units.

I initially thought the change could lead to some simplifications, but that thought was premature. (Whatever the input vectors are, they have to be rounded to the nearest available wavevector. So the wave vectors, in RLU and absolute units, need to be re-calculated no matter what the format of the input.)

A couple comments:

  1. Kipton suggested moving atom information into SpinInfo, which would sidestep some of the complexity of applying form factors. Probably it would be best to do this in another PR, possible the one bringing form factors into SWT calculations. I want to chat with Hao about that this next week.
  2. This PR starts to break down the internal convention of having q refer to RLU and k refer to absolute units. That seems fine, but if we want some new convention, we should agree on something.

kbarros and others added 4 commits August 6, 2023 17:09
It should have correct radius.
Also removes a FIXME; that q is for sure *not* in absolute units because it's calculated in RLU
and remove references to it from docs.

Remove options for selecting units from functions for creating binning parameters. All user-facing parameters should now accept exclusively absolute units.
@Lazersmoke
Copy link
Contributor

Essentially no change is required to the binning code, since absolute units were already the default input to the covectors matrix.

about connected_bath_bins:

There is some question about whether connected_path_bins should have a fixed density in RLU or in absolute units. Since the scattering vectors are uniformly spaced in RLU, it's more appropriate to space equally in RLU to get the maximum resolution. But at the same time, uniform spacing in absolute units is more physical in some sense. In short: RLU is optimal for Sunny data, whereas most experiment data will be in absolute.

Since connected_path_bins will not be called when using a BinningParameters derived from experiment data, it seems like it should be in RLU still! Perhaps it should accept the path in absolute units, though, but then the density parameter has an unnatural interpretation.

@Lazersmoke Lazersmoke merged commit 5b772ea into main Aug 7, 2023
3 checks passed
@Lazersmoke Lazersmoke deleted the absolute_q branch August 7, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants