Opened 8 years ago
Closed 7 years ago
#786 closed defect (fixed)
core_shell_parallelepiped 1-D model is incorrect
Reported by: | pkienzle | Owned by: | butler |
---|---|---|---|
Priority: | critical | Milestone: | SasView 4.2.0 |
Component: | SasView | Keywords: | |
Cc: | Work Package: | SasView Bug Fixing |
Description
The 1D core shell parallelepiped model does not use the thickness or SLD of the shell in the C dimension in the calculation so it cannot possibly be correct. Changing these parameters in the model does not affect the scattering curve.
Also, it does not support length_b = 0 in the core dimension, even if there is a shell thickness in b.
Attachments (1)
Change History (8)
comment:1 Changed 8 years ago by butler
- Owner set to butler
- Status changed from new to accepted
comment:2 Changed 8 years ago by butler
OK … so it turns out that this model was indeed originally written by Divya Singh as part of her thesis (appropriate appendix attached here) in 2008. It was reviewed and "approved" by Andrew Jackson for distribution in NIST IGOR macros in 2009. Looking at the code it seems several original bits that were wrong are commented out and replaced (presumably by Andrew)? more importantly, there is a comment at the beginning and next to several commented out sections that the current version does NOT support a shell in C. This explains the observed behavior reported in this ticket.
Miguel converted the code to the sasmodels framework and noted a number of oddities and commented the code but felt he did not understand the math enough to change it. Looking at it more closely I believe that in fact there must be an error in the normalization to units of length_b at least, and possibly a few other places.
Problem: how to validate the shells. for shells = 0 you should recover the parallelipiped or rectangular prism but that does not test the shells. Also note that notes in the original code and the original docs point out that A<B<C and that if they are not in that order the answer will be incorrect even though the calculation will return a value. So checking symmetric behavior is not an option either. It may be that someone needs to verify the math in the attached then implement and someone else independently verify the math and the implementation?
comment:3 Changed 8 years ago by butler
comment:4 Changed 8 years ago by butler
- Milestone changed from SasView 4.1.0 to SasView 4.2.0
- Priority changed from major to critical
We do not have time to do the comprehensive review necessary on this model for release 4.1 However this model has been available in the NIST IGOR macros since 2009. Any errors will be long standing. Rather than pull long available models we don't fully understand have added notes to documentation to warn user that the SasView team has yet to fully understand the code and that the code is under review. With that in place (and possibly also as release notes) this ticket can now move to 4.2
comment:5 Changed 7 years ago by dirk
The code for the core-shell parallelepiped is rewritten and it should be in-line with the math from the thesis appendix, which looks OK. Changes are here: https://github.com/SasView/sasmodels/commit/c69d6d6b7d1b4a8e1acc9c734f6ec3b26084cdd7. Trac did not catch my commit message with "ticket #786" in it.
comment:6 Changed 7 years ago by pkienzle
Still inconsistent with orientation average of the 2-D model, and furthermore code is inconsistent with the 1D integral used in hollow rectangular prism. A fix is implemented in PR #55 of sasmodels.
comment:7 Changed 7 years ago by GitHub <noreply@…>
- Resolution set to fixed
- Status changed from accepted to closed
Have tested this more and following the inheritance tree it seems that the code in the NIST IGOR package has exactly the same behavior. This should be a straightforward fix?