dnn should be lattice spacing for paracrystalline models (Trac #1156)

[pkienzle]

[Copied from ticket comment:4:ticket:805]

The parameter to the model is labelled as nearest neighbour distance, but I believe it is lattice spacing.

In units of lattice spacing, a, the q positions of the peaks are well determined by:

q = 2*pi/a * sqrt(h^2 + k^2 + l^2)

where SC allows all integer hkl, BCC requires h+k+l even and FCC require hkl all even or hkl all odd. Also need peak multiplicity to predict the peak height, since [100] = [-100] = [010] = [0-10] = [001] = [00-1], etc.

I checked peak position and height for dnn=20 and d_factor=0.06 for sc, bcc and fcc to verify the calculation.

We should rename dnn to length_a as in the parallelepiped model, and rename d_factor to lattice_distortion. Lattice distortion is actually proportional, Delta a / a, but distortion_proportion is too much of a mouthful.

Migrated from http://trac.sasview.org/ticket/1156

{
    "status": "assigned",
    "changetime": "2018-11-28T01:44:26",
    "_ts": "2018-11-28 01:44:26.486641+00:00",
    "description": "[Copied from ticket comment:4:ticket:805]\n\nThe parameter to the model is labelled as nearest neighbour distance, but I believe it is lattice spacing.\n\nIn units of lattice spacing, a, the q positions of the peaks are well determined by:\n\n   q = 2*pi/a * sqrt(h^2 + k^2 + l^2)\n\nwhere SC allows all integer hkl, BCC requires h+k+l even and FCC require hkl all even or hkl all odd. Also need peak multiplicity to predict the peak height, since {{{[100] = [-100] = [010] = [0-10] = [001] = [00-1]}}}, etc.\n\nI checked peak position and height for dnn=20 and d_factor=0.06 for sc, bcc and fcc to verify the calculation.\n\nWe should rename dnn to length_a as in the parallelepiped model, and rename d_factor to lattice_distortion. Lattice distortion is actually proportional, Delta a / a, but distortion_proportion is too much of a mouthful.",
    "reporter": "pkienzle",
    "cc": "",
    "resolution": "",
    "workpackage": "SasView Bug Fixing",
    "time": "2018-08-14T15:05:56",
    "component": "SasView",
    "summary": "dnn should be lattice spacing for paracrystalline models",
    "priority": "critical",
    "keywords": "",
    "milestone": "SasView 4.3.0",
    "owner": "butler",
    "type": "defect"
}

[pkienzle]

Trac update at 2018/09/11 21:51:47: pkienzle commented:

Renaming parameters requires too much work for the 4.2 release.

Could simply update the docs with the correct parameter description and leave any parameter renaming to 4.3.

[butlerpd]

Trac update at 2018/09/12 00:05:18: butler commented:

Actually Andrew started a branch on that but did not issue a PR ... partly because he made the changes but did not remember where the magic "conversion" file was that allows for backward compatibility.

[pkienzle]

Trac update at 2018/09/13 04:31:47: pkienzle commented:

Replying to [comment:2 butler]:

Actually Andrew started a branch on that but did not issue a PR ... partly because he made the changes but did not remember where the magic "conversion" file was that allows for backward compatibility.

Conversions go in sasmodels/convert.py.

Long term, may want to have version numbers for each model, and conversion functions within them. This would even work for plugin models.

This wouldn't handle model renaming, so still need convert.py or something like it.

[butlerpd]

Trac update at 2018/09/16 21:14:29: butler commented:

Looking at convert.py and conversion_table.py the infrastructure seems designed for translation from old style to new style models? Not at all clear to me how to use it for cases such as this? Assuming it can be used for such, some simple instruction somewhere might help?

[butlerpd]

Trac update at 2018/09/17 00:32:05: butler commented:

Actually reading the docs and then the code, the problems look like they go much deeper. I think dnn was in fact intended to be nearest neighbor NOT lattice spacing. Indeed the volume correction (ratio of sphere volume to unit cell volume) converts dnn (the nearest neighbor distance) to lattice spacing. NOTE: this seems to be done correctly in the code but is in incorrect in the BCC docs (both docs give the equation for FCC. SC also seems correct but then lattice and nearest neighbor are the same in this case). On the other hand the first peak position seems to suggest that dnn is the lattice spacing and the volume correction should only affect the intensity, though the P(Q) and distortion should move the peak position around. I suspect that whoever originally coded this model used the two (lattice and nearest neighbor spacing) interchangeably.

As noted by Michael Weir in an email:
Any chance there is confusion between parameters as Matsuoka paper uses "a_n" for nearest neighbour distance when you might reasonably expect "a" to refer to a lattice constant?

I believe the models need a complete going over and checked against the original references before going in and making changes. This probably means we need to move this to 4.3. Question would be if we change the vol ratio equation in BCC docs. We should add to Known Issues.

[RichardHeenan]

Trac update at 2018/09/17 09:18:57: richardh commented:

See also earlier comments and numerical issues in #155 and #904

Ought perhaps to for now put some large comments in the user docs as to where there may be issues.

[smk78]

Trac update at 2018/09/17 11:08:53: smk78 commented:

I was just logging in to say exactly that... the Known Issues is too obscure.

[pkienzle]

Trac update at 2018/09/17 14:37:29:

• pkienzle changed _comment0 from:

Replying to [comment:5 butler]:

Actually reading the docs and then the code, the problems look like they go much deeper. I think dnn was in fact intended to be nearest neighbor NOT lattice spacing. Indeed the volume correction (ratio of sphere volume to unit cell volume) converts dnn (the nearest neighbor distance) to lattice spacing.

In ticket:805#comment:4 point (6), I only checked the peak positions correspond to a rather than dnn. I did not check the volume fraction scaling factor.

Agreed that the code for volume fraction seems to be wrong.

• SC has one sphere per cell and should be V(sphere)/a^3^ = V(r/a).
• BCC has two spheres per cell and should be 2 V(r/a).
• FCC has four spheres per cell and should be 4 V(r/a).

to:

1537195365097595

• pkienzle commented:

Replying to [comment:5 butler]:

Actually reading the docs and then the code, the problems look like they go much deeper. I think dnn was in fact intended to be nearest neighbor NOT lattice spacing. Indeed the volume correction (ratio of sphere volume to unit cell volume) converts dnn (the nearest neighbor distance) to lattice spacing.

In ticket:805#comment:4 point (6), I only checked the peak positions correspond to a rather than dnn. I did not check the volume fraction scaling factor.

Agreed that the code for volume fraction seems to be wrong.

• SC has one sphere per cell and should be V(sphere)/a^3^ = V(r/a).
• BCC has two spheres per cell and should be 2 V(r/a).
• FCC has four spheres per cell and should be 4 V(r/a).

Also, need to understand whether returning form_volume as the volume of the sphere is correct when shape is scaled by volume fraction of lattice.

[pkienzle]

Trac update at 2018/09/25 23:40:15: pkienzle commented:

I updated the code in the ticket_1156 branch to use lattice spacing for computing volume fraction.

[pkienzle]

Trac update at 2018/09/25 23:58:47: pkienzle commented:

I extended the real space sampling program (explore/realspace.py in the ticket_1156 branch) to support sc, bcc and fcc lattices so I could try comparing directly to our models.

We will not be able to use this to cross check that we have the correct code for the paracrystalline models. In particular, we won't be able to use it to check that we have the volume normalization correct. It may be that I have the wrong code for locating the sphere centers in the lattice, or it may be that there are tricks for simulating an infinite lattice with a finite number of samples.

The following gets some peaks in the correct place (2D)

$PYTHONPATH=. python explore/realspace.py --type=sc --shuffle=0.06 --view=10,20,30 --spacing=2,2,2 --lattice=8,8,8 -d 2 -s 50000 sphere radius=20

but has large intensity at q=0 which doesn't appear in the models. Increasing the number of lattice points or the number of samples doesn't improve things much.

Similarly, the 1D case (-d 1) is merely suggestive of the correct form without matching the details very well.

[butlerpd]

Trac update at 2018/09/27 14:39:29:

• butler commented:

At this point all remaining tickets should either be blockers or critical. Critical would be those that do not stop the actual release but need to be dealt with soon after such as updating the marketplace. Tickets such as this are blockers in that if not fixed the docs need to have an appropriate warning in them for this release. Once that is fixed the ticket moves to 4.3 if it is not closed.

• butler changed priority from "major" to "blocker"

[butlerpd]

Trac update at 2018/09/27 14:39:53:

• butler changed owner from "" to "butler"
• butler changed status from "new" to "assigned"

[butlerpd]

Trac update at 2018/09/28 03:17:43: butler commented:

Looking at the original references, I believe the problem is that they do in fact mean nearest neighbor and are incorrect. We need to really think about this one I think before making changes.

[smk78]

Trac update at 2018/09/28 13:34:21: smk78 commented:

I have added a warning to the top of the doc strings for the bcc, fcc and sc paracrystal models that says:

Warning: This model and this model description are under review following concerns raised by 
SasView users. If you need to use this model, please email [email protected] for the latest situation.
The SasView Developers. September 2018.

I've also harmonised the formatting of the references across all three.

[butlerpd]

Trac update at 2018/09/28 17:12:26:

• butler commented:

Now that the warnings are added this can be moved to 4.3

• butler changed milestone from "SasView 4.2.0" to "SasView 4.3.0"
• butler changed priority from "blocker" to "critical"

[butlerpd]

Trac update at 2018/10/31 03:27:07:

• butler commented:

If we can figure out what the right answer is this should get fixed in 4.2.1 - it has nearly zero danger of causing unforeseen repercussions

• butler changed milestone from "SasView 4.3.0" to "SasView 4.2.1"

[butlerpd]

Trac update at 2018/11/28 01:44:26:

• butler commented:

There is no bandwidth to address this in 4.2.1 so moving to 4.3

• butler changed milestone from "SasView 4.2.1" to "SasView 4.3.0"

[butlerpd]

Some recent discussions with Andrew Whitten of ANSTO who was trying to use this model has managed to finally spur the full review of this issue as well as issue #155 that has been on the back burner for 3 years. The good news is I think we have now resolved all the issues and the bottom line is that there are some documentation errors and one error in the code itself.

The source of much confusion is the confusing nomenclature used in the 1987 paper used to code this model. There the authors define a set of 3 a_k vectors which are nearest neighbor vectors and a set of 3 orthogonal b_k vectors which are the primary cubic lattice vectors. The unfortunate nomenclature comes from the fact that they use scalar a_k to represent the magnitude of the a_k vector while they then state that the magnitude of the b_k vectors are all equal (true) and that magnitude is called a.

The original programmers and many of us who have tried to review this over the years have made the mistake of assuming that a_k and a were one and the same which is only true for the SC. Both a_k (i.e. d_nn) and a (lattice parameter) are used in different places in the equations copied from the paper. Thus it would be incorrect to call d_nn the scale parameter. There is in fact only one place in the code where the lattice parameter should replace d_nn.

A careful read of the code and the papers indicate that everything else seems to be coded correctly including both errors identified in the 1990 paper. Which brings us to the first error in the documentation: Z1,Z2,Z3 are taken from the 1987 paper BUT rewritten based on the 1990 correction (which is a separate correction from the correction of eq 1 in the 1990 paper). The second error is that the BCC equation for V_lattice is incorrect (it is actually just a copy of the FCC version). Thirdly, scale is NOT the volume fraction of spheres but the volume fraction of crystal in the system. Finally, V_lattice, which is a poor choice of nomenclature and should be changed, represents the volume fraction of spheres within the crystal. Thus scale * V_lattice is the total volume fraction of spheres. Unfortunately V_lattice is computed and used internally to the code only and not returned. Thus the user has no access to the volume fraction of spheres without a manual calculation.

SUMMARY

changes needed

• Calculate the lattice parameter (let’s call it b) from dnn in the xxx_zq(qa,qb,qc,dnn, d_factor) function.
• Replace dnn with the above in the cos terms (and only the cos terms) of the same function.
• Update the documentation as per above.

options for volume fraction

• Leave everything as is and make the user do manual computation if they want the vol fraction of spheres.
• Remove the V_lattice calculation altogether from the code (and documentation?). In this case scale becomes the total sphere volume fraction but the user will have to do manual calculations if they want to extract the vol fraction of spheres in the crystal and the vol fraction of crystal in the system.
• Add this to our increasing list of place where it would be nice to have the model return computed values.

[smk78]

In closing the above, @RichardHeenan notes in #155 (comment):

Would strongly suggest that the discussion in #284 is included as a footnote to the model documentation or in the code itself

[pkienzle]

I've extended realspace.py to simulate sc/fcc/bcc lattices. See #507