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

Ticket 1156: rename dnn to lattice spacing for paracrystalline models #97

Closed
wants to merge 19 commits into from

Conversation

pkienzle
Copy link
Contributor

@pkienzle pkienzle commented Mar 6, 2019

See Ticket #1156.

This code updates the name of the dnn parameter for lattice spacing. There are lots of questions about whether the paracrystal parameters are correct. The real space modeller has been extended to allow some experiments with monte carlo sampling, but this code may not be correct (see #1156:10).

I'm putting it as a PR so it doesn't get forgotten. It needs to be checked whether it performs as promised (renaming the parameter) irrespective of when the model itself is correct. If so, then it should be merged, because there are other tickets on paracrystal correctness (#805, #975, #1036).

@butlerpd
Copy link
Member

Why is this against the beta branch instead of master? Not sure I am comfortable with changing the name with all that entails in the support infrastructure for backwards compatibility till we actually are convinced we FULLY understand the model. I realize there is strong evidence that this is correct BUT I also know form painful experience that sometimes it is the unknown unknowns that bite us and in fact we may be completely misunderstanding everything.

@pkienzle pkienzle changed the base branch from beta_approx to master March 19, 2019 14:50
@pkienzle pkienzle changed the base branch from master to pak March 20, 2020 12:09
@pkienzle
Copy link
Contributor Author

I rebased this branch on PR #382, and fixed some errors in the lattice calculations.

Running the following:

python explore/realspace.py --qmax=1 --mesh=400 --type=sc --shuffle=0.03 \
    --lattice=40,40,40 --spacing=2,2,2 --samples=150000 sphere radius=25

eventually gives:

image

so at least for simple cubic the peak positions and heights are consistent with the corresponding lattice_spacing variable in the revised model. Similarly bcc and fcc give the main peaks, though they don't match the fine structure very well:

image

Some of this may be explained by finite lattice size (equivalent to low pass filtering) but it could also be due to errors in the simulation code or the analytic function.

I don't simulate paracrystals, only crystals with thermal distortion, so I'm not able to check that the lattice_distortion parameter is consistent.

@pkienzle
Copy link
Contributor Author

Check results against other software, such as scatter.[1]

[1] Förster, S., Apostol, L., Bras, W., 2010. Scatter: software for the analysis of nano- and mesoscale small-angle scattering. Journal of Applied Crystallography 43, 639–646. https://doi.org/10.1107/S0021889810008289

@pkienzle
Copy link
Contributor Author

From this link, a bcc crystal with lattice spacing 100 should have peaks at 0.088, 0.125, 0.154, 0.178. Checking with sascomp, the spurious detail in the sasmodels function (the orange line in the previous plots) vanishes when I increase the number of gauss steps:

./sascomp -linear -midq -ngauss=512 -nq=4000 bcc_paracrystal lattice_spacing=100 lattice_distortion=0.03 radius=0.05 background=0

the peaks are in the right place, but the intensities (48, 96, 24, 48) don't seem to agree:
image

@pkienzle pkienzle changed the base branch from pak to master March 21, 2020 09:53
Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in #284 (was trac#1156), this is not the correct fix. Once the maths from the paper are implemented correctly it would be interesting to run these tests again but given the age of this PR (particularly are far behind main it is at this point) and the fact that the needed corrections are much simpler as thy only require changing a few lines of code in 2 files, I recommend we close this PR without merging

@butlerpd
Copy link
Member

butlerpd commented Mar 1, 2022

Paul to check with @pkienzle before closing and deleting branch

@pkienzle
Copy link
Contributor Author

This PR is primarily about renaming the dnn to lattice_spacing and d_factor to lattice_distortion, which still seems like a reasonable request.

The only math change I see is a √1/2} scaling on fcc and √3/4} on bcc when computing radius for volume fraction.

@butlerpd
Copy link
Member

As discussed at the fortnightly call, this PR makes (incorrect) changes to the modes as well as changes to the simulation code only used for testing purposes so far. Thus I am only closing the PR but not the branch as @pkienzle wants to ensure that the simulation gets updated appropriately.

Note: the #284 discussion (dated Feb 25, 2022) points out the code in fact does need changing. simply replacing dnn with lattice_spacing will also be incorrect. In fact, both are used in the math, but the code only uses dnn. dnn is correct most of the time. However there is a single equation where dnn is used in the code which should in fact be lattice_spacing.

@butlerpd butlerpd closed this May 24, 2022
@pkienzle
Copy link
Contributor Author

pkienzle commented Nov 4, 2022

Simulation code is in #507 so I'm deleting the branch.

@pkienzle pkienzle deleted the ticket_1156 branch November 4, 2022 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants