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

Corrected cylinder docs #539

Merged

Conversation

butlerpd
Copy link
Member

Corrected equation one by removing the incorrect $\sin\alpha$ term in equation one. Also replaced P(q) with I(q) which is more correct in this case and eventually should be normalized across all models. Also updated the "last modified by" tag

Corrected equation one by removing the incorrect $\sin\alpha$ term in equation one. Also replaced P(q) with I(q) which is more correct in this case and eventually should be normalized across all models. Also updated the "last modified by" tag
@butlerpd butlerpd linked an issue Nov 10, 2022 that may be closed by this pull request
@@ -45,7 +45,7 @@

.. math::

P(q) = \frac{\text{scale}}{V}
I(q) = \frac{\text{scale}}{V}
\int_0^{\pi/2} F^2(q,\alpha) \sin \alpha\ d\alpha + \text{background}

Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to move the u-substitution comment down. That is,

P(q) = F²(q) =∫₀^π/2 F²(q, α) sin α dα

and so

I(q) = scale/V ∫₀^π/2 F²(q, α) sin α dα + background

After substitution with u = cos α, du = sin α dα,

P(q) = ∫₀¹ F²(q, u) du
I(q) = scale/V ∫₀¹ F²(q, u) du + background

Copy link
Contributor

@yunliu01 yunliu01 left a comment

Choose a reason for hiding this comment

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

The proposed changes are reasonable to me.
However, I feel that it may be useful to make some additional changes to the following line:
P(q)=F^2(q)=\int_{0}^{\pi/2}{F^2(q,\alpha)\sin(\alpha)d\alpha}=\int_{0}^{1}{F^2(q,u)du}

It may be better to change it to
P(q)=F^2(q)=\int_{0}^{\pi/2}{F^2(q,\alpha)\sin(\alpha)d\alpha}

The concern is that F^2(q, u) may confuse people.
We already used F^2(q,\alpha) as the form factor.
When using F^2(q, u), we implied that u = \alpha. However, in fact, u = cos(\alpha).

@pkienzle
Copy link
Contributor

Looking at the code, the cylinder model does not use a u-substitution for the integral. It could, and similar functions do, but it does not.

Actually, there is no need in this documentation really to go into the numerical integration detail even it if were used. But since it is not I just removed the offending line.
@butlerpd butlerpd merged commit 7137223 into master Jan 9, 2023
@butlerpd butlerpd deleted the 495-equation-one-in-cylinder-model-documentation-is-incorrect branch January 9, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Equation one in Cylinder model documentation is incorrect.
4 participants