-
Notifications
You must be signed in to change notification settings - Fork 116
fix #155 refactor(database): standardize Hall symbols #317
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
Hmm, I think we should add the database generation to the CI, can someone point me to the steps for that? The database folder could also use some refactoring |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## develop #317 +/- ##
========================================
Coverage 83.60% 83.60%
========================================
Files 23 23
Lines 7942 7942
========================================
Hits 6640 6640
Misses 1302 1302
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
I'm not very familar with CI. Do you mean you want to generate the database automatically? I think it is a little difficult.
For example,
The output is just Lines 8495 to 9255 in 9957bfc
after some modification. In fact, the database does not need to be generated by the user. Do we need to add it to the CI? |
Yes it should be added to check PRs like these. What kind of manual editing do you do?
Exactly because of that it should be in the CI. Like a pre-commit. |
Ok, then this PR only used Lines 1107 to 8495 in 9957bfc
python hall2operations.py --ref spg.csv gives the range of each hall symbols' symmetry operations static const int symmetry_operation_index[][2] , i.e. {num_operations, start_index} of each group in symmetry_operations[] Lines 9258 to 9788 in 9957bfc
These two output can be directly copied to spg_database.c after code formating. Then for layer group, the symmetry operations are added after the ones of space groups in symmetry_operations[] . So the start index need to be shifted. spglib/database/hall2operations.py Line 344 in 9957bfc
and spglib/database/hall2operations.py Line 420 in 9957bfc
count = 7388 .python hall2operations.py --dump layer_spg.csv gives the rest of symmetry_operations[] Lines 8497 to 9254 in 9957bfc
python hall2operations.py --ref layer_spg.csv gives static const int layer_symmetry_operation_index[][2] Lines 10030 to 10146 in 9957bfc
Lines 40 to 1103 in 9957bfc
However, this code does not support layer group now. So I generate Lines 9791 to 10027 in 9957bfc
manually (excel again). If you want to automate it. An |
For the most part they should still work, but they really need to be updated: Best is to just migrate them into the already provided python package and fix the issues there. Other than that there are no manual formatting issues or anything else? |
I'm not sure. One thing is that some of the codes are written in python 2. Did that cause the errors? |
I have confirmed spg.csv and layer_spg.csv. |
If this PR changes the output of the Hall symbol, what is the best way to inform it to users? I think writing it in the release not is enough though, but not just as a commit log. |
I think a description in the release note will be the best way. |
I updated the 3D space group Hall symbols to be the same as the ones in ITB (2010). I also modified the layer group Hall symbols. Now every layer group Hall symbol (except 20, 52:1, 54 and 64) has a space group counterpart in ITB. They share the same symbol, except we use lowercase 'p' and 'c' to represent layer group. The four symbols are different because the origin choice are different in ITA and ITE, as shown in the following table and diagram. The layer group Hall symbol table is added to Appendix D in the overleaf.
@atztogo, @lan496 could you take some time to double check this as there are too many entries and is very error-prone.
Fixes #155