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

[Release 1.0.7] model updates #544

Merged
merged 15 commits into from
Jan 27, 2023
Merged

Conversation

butlerpd
Copy link
Member

This cherry picks changes to two types of models.

  • Cylinder
    • change the docs so that equation 1 is correct
    • Made a few other changes to make reading more clear
  • Paracrystal models (all 3)
    • Fixed code for FCC and BCC
    • Fixed docs for FCC and BCC
    • Updated docs for FCC, BCC, and SC with the full equations used from the papers
    • Removed the warning that the model might be wrong.
      The second bullet fixes a long standing problem with the paracrystal models (not just the documentation)

butlerpd and others added 15 commits January 22, 2023 23:28
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
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.
This should fix the BCC calculation. Still need to fix FCC and then the documentation to both.
This should fix the BCC and FCC calculations. The documentation is also fixed. Mostly this is from Jonathan Gaudet's work but some of the documentation has been updated by me.
Updated the BCC docs with corrections but more importantly adding the full description of the equations used as given by jonathan Gaudet in his latex writeup.  Also fixed the unit test, but it would be helpful to have an independent assesment of the correct answers. At which point some better unit test coverage could be provided. Note that this change drops the intensities by about half!
Fixed the unit test, but it would be helpful to have an independent assesment of the correct answers. At which point some better unit test coverage could be provided.
updated comments
This should now be correct. However I do need to fix all docs to remove referene to |f_0|^2 which is just the P(q). that will be another commit however.
Removed spurious reference to |f(0)|^2 which is just the structure factor P(q). The equation was thus essentially already given at the beginning and would have just been confusing with no real information content.
Copy link
Collaborator

@krzywon krzywon left a comment

Choose a reason for hiding this comment

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

Previously reviewed. No changes between the review and here.

@krzywon krzywon merged commit 09d0b64 into release_1.0.7 Jan 27, 2023
@butlerpd butlerpd deleted the _RELEASE_1.0.7]_model_updates branch February 3, 2023 19:17
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.

2 participants