Fix DiffractionPattern k values/vectors #777
Conversation
bdice
changed the title
Codecov Report
@@ Coverage Diff @@
## master #777 +/- ##
==========================================
- Coverage 55.66% 55.57% -0.10%
==========================================
Files 16 16
Lines 2443 2438 -5
Branches 35 33 -2
==========================================
- Hits 1360 1355 -5
Misses 1070 1070
Partials 13 13
Continue to review full report at Codecov.
|
I haven't decided on a final scope for this pull request. I will discuss with @AKerr9509 and we will make a plan. Separating the to-do list at the top of this PR description into a series of smaller changes may be best. I propose the following:
|
|
In a future PR, we're also going to change the normalization criteria such that |
Prior to this commit, the units of the diffraction plot were in pixels...calling something like `ax.axhline(2*np.pi)` would put a horizontal line on the 6th or 7th row of pixels from the top. This commit changes that behavior so that the units in the image space correlate to units in k-space...now calling `ax.axhline(2*np.pi)` will put a horizontal line at y = 2*pi. This enables easy drawing on the plots, which may be useful, e.g., to make sure peaks in s(k) correspond to the actual lattice spacing in a crystalline system. This change does not affect using the diffraction class to find where the bright spots are, as that is more accurately done using the `k_vectors` attribute of the class.
for more information, see https://pre-commit.ci
… ends of each array. Calculations used for comparison are based on np.fft.fftfreq. Any feedback on style and correctness is appreciated, in particular whether it is worth it to add tests for correct k-vectors at indices [0, -1] and [-1, 0] in addition to [0, 0] and [-1, -1].
…to fix/k-values
for more information, see https://pre-commit.ci
…s suggestions. With quite a few tests that are identical except for slight changes to indices, I am wondering if there is someway to put this code in a loop.
for more information, see https://pre-commit.ci
…ple cubic system. It is sensitive to certain parameters right now; the test registers peaks best when sigma_noise = 0.1*lattice_length and vq.kmeans is only passed indices where structure factor is in the greatest 50 values.
…to fix/k-values
for more information, see https://pre-commit.ci
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
…tem with various grid size, output size, and zoom values. It isolates the brightest areas of the diffraction pattern (S(q) greater than a certain threshold), and verifies that the ideal peak locations given by k * R = 2 * pi * N are contained in these areas. I opted for this approach over locating peaks with scipy's kmeans algorithm because the random noise in the system and slight inexactness due to binning prevented kmeans from reliably locating the exact pixels which contained diffraction peaks. Parts of this test feel a bit 'hacky' right now (using a dictionary, iterating through it twice), and I am open to any/all style and efficiency suggestions.
…to fix/k-values
for more information, see https://pre-commit.ci
…to fix/k-values
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
A few comments for improvement. @AKerr9509 it seems like you're on the right track. Some tests are failing, take a look at the output from CI and you can see where your code isn't working (some variables aren't defined because you changed some of the names but not all). Ping me on Slack if get stuck or want help with anything.
|
@AKerr9509 Good work on this! I'm going to merge since this fixes bugs and is at a good stopping point. We'll start a new pull request for follow-up work. |
Description
This PR rescales k values and k vectors so that they correspond to the usual conventions in condensed matter physics.
This corrects the calculation of k values/vectors such that the peaks in the image should now correspond to G vectors that satisfy the following relationship for any lattice vector R (from Wikipedia):
However, this has only been tested in a limited case (simple cubic system in a cubic box) and needs further testing. I am concerned that the current calculation using
np.max(system.box.to_matrix())is incorrect for boxes like{"Lx": 10, "Ly": 10, "Lz": 20}when viewing down the z-axis. It should probably be based on the plane used to project the points (the box face most aligned with the view axis).I also changed the behavior of
zoom, please see screenshots below for a description.The notebooks used to test this behavior (and develop the correct scaling factors for NumPy's FFT frequencies) are attached here.
fft_tests.zip
To-do:
Once this is resolved, then we can investigate the problems in issue #750.
Motivation and Context
Resolves: #723
How Has This Been Tested?
Tests are needed. See to-dos above.
Screenshots
Previously, the constructor argument
grid_sizewas divided by the compute argumentzoom, and the resulting value was used as the "true" grid resolution of the histogram image and thus the FFT of that histogram. I experimented with changing this so that thezoomparameter does not affect the histogram resolution. Here is a screenshot of how that affects the behavior:Truncated resolution, old behavior:
grid_size/zoomzoom = 4, grid_size = 512, output_size = 512
zoom = 2, grid_size = 512, output_size = 512
Full resolution, new behavior:
grid_sizezoom = 4, grid_size = 512, output_size = 512
zoom = 2, grid_size = 512, output_size = 512
The difference is that the truncated resolution diffraction patterns appear to decay more quickly. I'm unsure about this, so I changed the code's behavior to use the full
grid_sizeresolution.Types of changes
Checklist: