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

Documentation error in core_shell_sphere model #578

Open
smk78 opened this issue Aug 17, 2023 · 4 comments
Open

Documentation error in core_shell_sphere model #578

smk78 opened this issue Aug 17, 2023 · 4 comments

Comments

@smk78
Copy link
Contributor

smk78 commented Aug 17, 2023

User StevenP emailed to say:

Been checking through the multi shell modelling and I think something is wrong in the documentation in the pre factor. I tried to document my steps but I can’t seem to get the documentation to agree with the Pederson formalism eqn 57 [1].

Hopefully I made a simple error. I did not delve into the c code just the documentation. Also the original 1955 reference I checked and it seems to be one of those references that leads to a dead end.

Wonder if you can spot something? BillH quickly checked it and agrees with my schoolboy maths.

[1] Advances in Colloid and Interface Science, 70 (1997) 171-210
SParnell_CoreMultiShell_Query

@smk78
Copy link
Contributor Author

smk78 commented Aug 17, 2023

LucasW comments:

They don’t seem to specifically say what the issue is, but my guess is that Vs is just not the scattering mass, essentially there’s no rho term.

Some dimensional analysis of the docs ought to sort this out.

From the first equation:

In the docs, if “background” has dimension 1/L and “scale” is dimensionless, P(q) must have dimension 1/L, and so F(q) squared should have dimension L^2.

If F^2(q) has dimension L^2, then F(q) should have dimension L.

From the second equation:

The fractions with the sin functions in are dimensionless which means that the dimension of the square brackets is [V][sld] = L^3 / L^2 = L.

The Vs term has dimension L^3, meaning the according to the equation, the dimension should be 1/L^2.

This is clearly a contraction.

So I agree something is wrong here.

Paper

Looking at the equations for the core shell F(q) in the linked paper, it is is dimensionless

This seems pretty consistent with the rest, particularly eqn 46. Dimensions of dsigma/dOmega should be area, which makes P = F^2 dimensionless.

Code

The function core_shell_fq in core_shell.c returns values with dimensionality of L. As does the randomly picked capped_cylinder.c. This is at least consistent with the implicit definition of the first equation in the core_shell docs.

Summary

  1. For the docs to be right, I think we need to get rid of the Vs term. But maybe someone else can check this is correct.
  2. The paper has a different definition of F(q) to that used in SasView.
  3. I think the difference between these two definitions is describing things in terms of volumes and describing them in terms of particle numbers - I know there’s some details about this that others are more familiar with.

@smk78
Copy link
Contributor Author

smk78 commented Aug 17, 2023

PaulB comments:

The short answer is yes – I believe you are correct Lucas – the F(Q) should not be divided by V. This is one of many model docs that need to be reviewed (I note that this one, like many others, doesn’t even have an author or reviewer etc!).

The longer answer is that this is an example of what I thought was a long standing ticket to review all the model documentation (but I cannot find it). All documentation needs to be reviewed for consistency in style and nomenclature, a lot of it needs expanding and those that need expanding probably need a careful review of the math as well. I found a typo last year in the cylinder model (a very basic model if ever there was one).

On the other hand there are quite a few that are well documented and I suppose we should hunt some down to suggest as templates for people.

One of the biggest normalizations is the way we write the first equation: I(Q)=xxxx. I remember discussing this back when I had to fix the cylinder model but may only have put those thoughts in the ticket that was then closed? (that would have been silly but not unlikely ☹️).

People have variously used P(Q) to mean I(Q) like here, or I(Q) and I think maybe even d\Sigma/d\Omega. Some include the background term and some do not (even though it is always included in our models). Of course part of the latter could be because they are using P(Q) which, if used to include the “prefactors” (and that is sometimes done) it should not include background.

MY PROPOSAL for standardizing equation 1:
For that equation my proposal was that we standardize on I(Q) for the left side of the equation (on the grounds that d\Sigma/d\Omega implies absolute scale and we do not require that), that all equations must add the background term (since all models get that automatically) all use P(Q)=F(Q)^2 (or F(Q) * (F^*(Q)) and that we keep the scale/V term at the front of the prefactor because that is how the “magic normalization” is done to provide number average results (basically that is what sasmodels does with form_volume). This last of course means that one must write the rest of the equation so that there is a V^2 in the P(Q). Essentially a slight variation of Pederson's equation 46 where nV/V = scale (volume fraction) divided by volume. Note that for models with multiple slds the V^2 and rho^2 usually get subsumed into P(Q) as was done here... assuming my brain is functioning and i got it the right way round this time.

@lucas-wilkins
Copy link
Contributor

As for the factor of 3, as long as it's there somewhere it doesn't matter.

@butlerpd
Copy link
Member

The factor of 3 should be correct (eventually getting squared to be 9). Agreed that where it shows up should be for best readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants