-
Notifications
You must be signed in to change notification settings - Fork 116
Fix a bug in layer groups. #199
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
…ic_axes array. Caused segfaults.
Update: Using an online tool, if I cut spacegroup 38, I get layergroup 35. Thus, if I change (lg 35 -> sg 35, to lg 35-> sg 38), along with the tolerance update: all 15733 materials pass the sanity check. http://dcwww.camd.dtu.dk/~kuisma/spacegroup38.png With these changes, the data is here: |
Thanks @mikaelkuisma1 for your bug fix along the detailed explanation (helpful!), and systematic test. How did you run over C2DB 15000 data systematically? Currently I am busy. I would like to have time to look at it next week or later. But I have not yet been very familiar with the layer group, and so I need also some basic study on it, too. @site-g, can you have a look at it? Probably we need to refactor the code, too. @lan496, do you have any comment? (also magnetic layer group for the future) |
|
(Just a comment) For testing, we may need another repository to check layer-group implementations. For example, I've created and checked magnetic-space-group implementations for MAGNDATA in a private repo. For refactoring, wrapper functions to hide comparison of |
Codecov ReportBase: 85.12% // Head: 85.12% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## develop #199 +/- ##
========================================
Coverage 85.12% 85.12%
========================================
Files 16 16
Lines 1318 1318
========================================
Hits 1122 1122
Misses 196 196 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
That sounds very helpful for avoiding typo! In fact, the layer-group implementation is far from being completed. There requires:
And also some codes need to be refactored, but I have being very busy since the end of last year and contribute nothing to the project. Very sorry. |
@site-g, no need to be sorry. Just providing some comments is helpful. So can I merge this PR? Then we will discuss on the layer group in a new issue. |
I think we can, this is a typo. |
Merged. Thanks @mikaelkuisma1. |
I run the layer group detection to all 15000 2D-materials in C2DB. This resulted in 22 Bus errors or segfaults.
Here is a simple script modified from example.c to reproduce the problem.
gcc runlayers.c -I../include -L../_build -O0 -lsymspg -fsanitize=address -static-libasan -ggdb && LD_LIBRARY_PATH=
pwd
/../_build ./a.outI traced the error down to
cel_layer_is_overlap
function, which typically had periodic axes 0, 1, until they were 2 1070703038, and the code crashed. It is quite miraculous that the code worked before, probably due to some stack alignment of previous calls, that the periodic axes happened to be 0 and 1.This merge request allows calculation for those 22 systems, while keeping all other results the same. However, I am still not sure are the results correct.
There are some more bugs remaining still. Edit: all of these might be resolved, see comment below.
periodic axes are hard coded in some places, however, I do not know if this is related
(Mapped according to this table) https://arxiv.org/pdf/1306.1280.pdf
Here LG is layergroup, and SG is spacegroup, and mappedsg is mapping of layer group to spacegroup by the table.
https://pastebin.com/sUqCtu4j
Most of them are predicted to be layergroup 35, according to article, spacegroup 35, but predicted to be spacegroup 38.