Skip to content

Conversation

singularitti
Copy link
Contributor

@singularitti singularitti commented Nov 9, 2023

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

@singularitti singularitti changed the title Fix example errors in definition.md Fix example wrong results in definition.md Nov 9, 2023
@singularitti
Copy link
Contributor Author

singularitti commented Nov 9, 2023

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.

@LecrisUT
Copy link
Collaborator

LecrisUT commented Nov 9, 2023

Please use centering as you've done in this PR. If you can find any similar typos that would be great. Also would be great if they were different commits. Thank you very much for catching these.

@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 asnan. I think it's a flag needed to specify using static sanitizers

@LecrisUT LecrisUT requested a review from atztogo November 9, 2023 11:04
Copy link
Member

@lan496 lan496 left a 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
Copy link
Member

@lan496 lan496 Nov 9, 2023

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?

Copy link
Collaborator

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).

Copy link
Collaborator

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.

int ref_find_similar_bravais_lattice(Spacegroup *spacegroup,

@atztogo
Copy link
Collaborator

atztogo commented Nov 10, 2023

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?

@singularitti
Copy link
Contributor Author

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".

@LecrisUT
Copy link
Collaborator

@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

@singularitti
Copy link
Contributor Author

@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.

@lan496
Copy link
Member

lan496 commented Nov 10, 2023

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?

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:
https://github.com/search?q=repo%3Aspglib%2Fspglib+centering&type=code
https://github.com/search?q=repo%3Aspglib%2Fspglib%20centring&type=code

@LecrisUT
Copy link
Collaborator

@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.

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.

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?

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: https://github.com/search?q=repo%3Aspglib%2Fspglib+centering&type=code https://github.com/search?q=repo%3Aspglib%2Fspglib%20centring&type=code

For me I find the spelling centring a bit jarring, particularly since I am more used to the AE pronunciation. The other way around, I don't think it is as problematic

@singularitti
Copy link
Contributor Author

singularitti commented Nov 11, 2023

While we are at it, could you please also check this equation?

\boldsymbol{P} = \begin{pmatrix}
\frac{1}{2} & \frac{1}{2} & 0 \\
\frac{\bar{1}}{2} & \frac{1}{2} & 0 \\
0 & 0 & 1
\end{pmatrix}.
image

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`.

@atztogo
Copy link
Collaborator

atztogo commented Nov 11, 2023

@singularitti, thanks. It is incorrect. It must be transposed as below,

\boldsymbol{P} = \begin{pmatrix}
\frac{1}{2} & \frac{\bar{1}}{2} & 0 \\
\frac{1}{2} & \frac{1}{2} & 0 \\
0 & 0 & 1
\end{pmatrix}.

@singularitti
Copy link
Contributor Author

Agreed. I can update it by the way.

@atztogo atztogo merged commit 9ab50a4 into spglib:develop Nov 13, 2023
@atztogo
Copy link
Collaborator

atztogo commented Nov 13, 2023

Thanks @singularitti!

@singularitti
Copy link
Contributor Author

My pleasure to help!

@singularitti singularitti deleted the patch-1 branch November 13, 2023 08:10
@LecrisUT LecrisUT added this to the 2.2 milestone Jan 25, 2024
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.

4 participants