-
Notifications
You must be signed in to change notification settings - Fork 116
Fix example wrong results in definition.md #355
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
Conversation
For the "centrng" typo, it seems like you chose the "centring" in your docs, while I wrote "centering", more in line with American English habits. Both are working English words. I could change it to "centring" if you prefer. |
Please use @atztogo @lan496 I leave it to you to comment on the validity of the contents. I will look a bit into why the windows builders do not find |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@singularitti Thank you for your careful checks and corrections!
1 0 0 | ||
0 1 0 | ||
0 0 1 | ||
Origin shift: 0.000000 0.000000 0.000000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atztogo I am curious about why the origin shift has changed. Maybe related to the axis choice for orthogonal space groups based on Euclidean normalizers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are correct. a(0, 0, 0) and b(1/2, 1/2, 1/2) are in the same Wyckoff set. Plus centring, we get (0, 0, 1/2).
https://www.cryst.ehu.es/cgi-bin/cryst/programs/nph-normsets?from=wycksets&gnum=64
In the current implementation of spglib, we can not distinguish them, although it would be more natural to give the origin shift (0, 0, 0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following comment is unrelated to this PR. I leave it because it came to my mind.
I probably tried to find the shortest (under some condition) origin shift in this function.
Line 1415 in a0263dd
int ref_find_similar_bravais_lattice(Spacegroup *spacegroup, |
Thanks @singularitti catching typos. I am not good at English, but I tend to use "centring" following the International Table for Crystallography. In literature, I find both, so no problem which is used. How do you think, @lan496? |
Hi @atztogo, "centring" and "centering" are both valid English words; the former is British, and the latter is American. I can change it to "centring". |
@singularitti, do you want to handle #359 in this PR? Seems pretty straightforward. Not sure whereto add the tests though, to rtd or to pytes |
@LecrisUT, do you mean convert these codes I modified into doc tests? Sure! But it might not be instantaneously done since I've been pretty busy recently. |
The IUCr dictionary shows both "centre" and "center". So, I also think both will be fine. Actually, I did not notice ITA tends to use the former. Considering to follow the ITA first, I guess "centring" is better. I find the current spglib mixes the both: |
I can handle these tomorrow. I want to add spell and link checks while we're at it anyway. You're ok with me merging your commits and adding more on top of that.
For me I find the spelling |
While we are at it, could you please also check this equation?
If I understand the symbols correctly, shouldn't the correct matrix be its transpose? Then you could have $\mathbf{b} = \frac{1}{2}\mathbf{a}_s +\frac{1}{2}\mathbf{b}_s`. |
@singularitti, thanks. It is incorrect. It must be transposed as below,
|
Agreed. I can update it by the way. |
Thanks @singularitti! |
My pleasure to help! |
The example code given in docs behave differently compared to the results from the latest version of
spglib
. Please check whether the results was wrong or there are bugs in the code.Spglib
version: 2.1.0