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

Incorrect SpaceGroup symbol attribute for 16 space group types #3845

Closed
kaueltzen opened this issue May 24, 2024 · 7 comments · Fixed by #3859
Closed

Incorrect SpaceGroup symbol attribute for 16 space group types #3845

kaueltzen opened this issue May 24, 2024 · 7 comments · Fixed by #3859
Labels

Comments

@kaueltzen
Copy link
Contributor

Python version

3.10

Pymatgen version

2024.5.1 or master

Operating system version

No response

Current behavior

output of minimal example:

48 Pnnn1
50 Pban1
59 Pmmn1
68 Ccce1
70 Fddd1
85 P4/n1
125 P4/nbm1
126 P4/nnc1
129 P4/nmm1
130 P4/ncc1
201 Pn-31
203 Fd-31
222 Pn-3n1
224 Pn-3m1
227 Fd-3m1
228 Fd-3c1
16

Expected Behavior

For the minimal example provided below I would expect len(err) to be zero, meaning that every SpaceGroup symbol attribute provides a valid string that can be used to instantiate again a SpaceGroup object.
However, this is not the case for 16 space group types (see above). They all differ from their correct Hermann Mauguin symbol by an additionally appended "1" (resulting even in an impossible fourth symmetry direction / blickrichtung for most of them).

The symbol attribute is set here

self.symbol = re.sub(r":", "", re.sub(r" ", "", spg["universal_h_m"]))

I am not sure what the "universal" Hermann Mauguin symbol ("universal_h_m" key) and the additional :1 represents and why the key "hermann_mauguin" is not used instead, but the current code gives wrong space group symbols in 16 cases.

Minimal example

from pymatgen.symmetry.groups import SpaceGroup

err = []
for sg_num in range(1, 231):
    sg = SpaceGroup.from_int_number(sg_num)
    sg_symbol = sg.symbol
    try:
        SpaceGroup(sg_symbol)
    except ValueError as e:
        print(sg_num, sg_symbol)
        err.append(sg_num)

print(len(err))

Relevant files to reproduce this bug

No response

@kaueltzen kaueltzen added the bug label May 24, 2024
@JaGeo
Copy link
Member

JaGeo commented May 24, 2024

It affects the Materials Project website as well.

@JaGeo
Copy link
Member

JaGeo commented May 27, 2024

Before @kaueltzen starts to correct it, we would like to ask for feedback on this as this will be a breaking change with potentially major effects. The symbol is clearly wrong. Nevertheless, people work with the wrong symbol.

@shyuep @janosh, thoughts?

Any thoughs from MP side: @munrojm @tschaume @yang-ruoxi ?

Thanks in advance.

@JaGeo
Copy link
Member

JaGeo commented May 29, 2024

@shyuep ?

@JaGeo
Copy link
Member

JaGeo commented May 30, 2024

We will start fixing it tomorrow as it is a major bug. Please let us know by end of this day if you object.

@tschaume
Copy link
Member

@JaGeo thanks for tagging us. We're good with the change from MP's side.

@kaueltzen
Copy link
Contributor Author

Actually, there are still open questions and I would appreciate feedback on them by @shyuep or @janosh :

If we decide on the standard Hermann-Mauguin symbol as the symbol attribute, the seven rhombohedral space group types would not contain information on their setting anymore (R-centered hexagonal cell or primitive rhombohedral cell). In this case, the current implementation of the is_compatible() method would not behave as previously as the appended ":H"/":R" is not part of the Hermann-Mauguin symbol and all rhombohedral space group types would always be evaluated as in the rhombohedral setting.
One solution could be to add a hexagonal bool parameter to the init of SpaceGroup as it is already done in the classmethod from_int_number(). What do you think?

Also, I have a question regarding the init:

self.point_group = spg["schoenflies"]

The point_group attribute may be set as spg["schoenflies"]. However, this key seems generally to refer to the Schoenflies space group (not point group) notation. By removing the superscript, one would arrive at the corresponding point group symbol and I would suggest this change.

Also, regarding SYMMOPS.json: the key "crystal_class" does not refer to the actual crystal class but for some entries to the crystal system, for some entries to the lattice system (or both if identical). Although this key is not used right now, I would highly recommend correcting it in case it is going to be used in the future and decide on whether to include a crystal system or lattice system key. What are your thoughts?

Thanks in advance (:

@JaGeo
Copy link
Member

JaGeo commented Jun 3, 2024

Hi all,

We are also happy to just correct these issues according to @kaueltzen 's suggestions. It will, however, be a breaking change. But as this seems wrong, we should correct it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants