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.

Sign up for GitHub

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

SpaceGroup changes #3859

Merged
merged 25 commits into from
Jul 14, 2024
Merged

SpaceGroup changes #3859

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ebc35f0
Replaced SpaceGroup symbol attribute with its Hermann-Mauguin symbol,…
kaueltzen Jun 4, 2024
fe7363d
Removed crystal class key from symm_ops.json.
kaueltzen Jun 4, 2024
ebe1246
Merge branch 'materialsproject:master' into space_group
kaueltzen Jun 4, 2024
561dfef
Test for correct setting of hexagonal attribute when instantiating fr…
kaueltzen Jun 5, 2024
25f90e7
Noted change and replacement option for SpaceGroup symbol in compatib…
kaueltzen Jun 5, 2024
d0c50ba
Added from_space_group class method to PointGroup, added tests
kaueltzen Jun 6, 2024
10b5a14
Added mapping to standard setting in PointGroup.from_space_group(), m…
kaueltzen Jun 6, 2024
0eea0bd
Merge branch 'master' into space_group
kaueltzen Jun 6, 2024
c008de9
Updated core/test_surface.py to assign lattice as in SpaceGroup is_co…
kaueltzen Jun 6, 2024
0b1dd3c
Modified databases and SpaceGroup init to ensure compatibility with n…
kaueltzen Jun 6, 2024
39d3683
Added tests for issue #3862, modified full_symbol and point_group att…
kaueltzen Jun 6, 2024
b6d6c25
Modified PointGroup.from_space_group() to also handle symbols with id…
kaueltzen Jun 7, 2024
6a089c7
Added test for warning if SpaceGroup.full_symbol is not available.
kaueltzen Jun 7, 2024
ccc4cc8
Removed warning test.
kaueltzen Jun 7, 2024
828cf5d
Merge branch 'materialsproject:master' into space_group
kaueltzen Jun 10, 2024
dd60f90
tweak incompat warning
janosh Jun 10, 2024
1d744b9
add test_full_symbol_warning
janosh Jun 10, 2024
15acd36
add author + date to dev_scripts/update_spacegroup_data.py
janosh Jun 10, 2024
6439fed
typos
janosh Jun 10, 2024
a33cc3c
warning occurs only once, move test_full_symbol_warning up as workaro…
janosh Jun 10, 2024
81f76f4
Updated compatibility.md to also handle old symbol replacement of P2_…
kaueltzen Jun 12, 2024
26e7728
Merge branch 'master' into space_group
kaueltzen Jul 5, 2024
56416f2
pre-commit auto-fixes
pre-commit-ci[bot] Jul 5, 2024
21777c7
Updated dev script path to new src layout.
kaueltzen Jul 5, 2024
adcff10
Merge branch 'master' into space_group
JaGeo Jul 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions pymatgen/symmetry/groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class SpaceGroup(SymmetryGroup):
translations: ClassVar = {k: Fraction(v) for k, v in SYMM_DATA["translations"].items()}
full_sg_mapping: ClassVar = {v["full_symbol"]: k for k, v in SYMM_DATA["space_group_encoding"].items()}

def __init__(self, int_symbol: str) -> None:
def __init__(self, int_symbol: str, hexagonal: bool = True) -> None:
"""Initialize a Space Group from its full or abbreviated international
symbol. Only standard settings are supported.

Expand All @@ -209,9 +209,22 @@ def __init__(self, int_symbol: str) -> None:
possible settings for a spacegroup, use the get_settings()
classmethod. Alternative origin choices can be indicated by a
translation vector, e.g. 'Fm-3m(a-1/4,b-1/4,c-1/4)'.
hexagonal (bool): For rhombohedral groups, whether to handle as in
hexagonal setting (default) or rhombohedral setting.
If the int_symbol of a rhombohedral spacegroup is given with the
setting ("(:)H"/"(:)R"), this parameter is overwritten accordingly
(please note that the setting is not contained in the symbol
attribute anymore).
"""
from pymatgen.core.operations import SymmOp

if int_symbol.endswith("H"):
self.hexagonal = True
elif int_symbol.endswith("R"):
self.hexagonal = False
else:
self.hexagonal = hexagonal

int_symbol = re.sub(r" ", "", int_symbol)
if int_symbol in SpaceGroup.abbrev_sg_mapping:
int_symbol = SpaceGroup.abbrev_sg_mapping[int_symbol]
Expand All @@ -223,13 +236,13 @@ def __init__(self, int_symbol: str) -> None:
for spg in SpaceGroup.SYMM_OPS:
if int_symbol in [spg["hermann_mauguin"], spg["universal_h_m"]]:
ops = [SymmOp.from_xyz_str(s) for s in spg["symops"]]
self.symbol = re.sub(r":", "", re.sub(r" ", "", spg["universal_h_m"]))
self.symbol = spg["hermann_mauguin"]
if int_symbol in SpaceGroup.sg_encoding:
self.full_symbol = SpaceGroup.sg_encoding[int_symbol]["full_symbol"]
self.point_group = SpaceGroup.sg_encoding[int_symbol]["point_group"]
else:
self.full_symbol = re.sub(r" ", "", spg["universal_h_m"])
self.point_group = spg["schoenflies"]
self.full_symbol = spg["hermann_mauguin"] # TODO full symbol not available from spg->maybe not add?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate on this TODO? does this mean the current full_symbol is incorrect or incomplete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - as far as I can see, the full symbol (e.g., F4/m-32/m instead of Fm-3m) is only contained in SpaceGroup.sg_encoding, but not in SpaceGroup.SYMM_OPS. Both spg keys "hermann_mauguin" and "universal_h_m" do not contain this information. We could still set this attribute but issue a warning (maybe that's better than not setting it in these cases?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a warning and this seems to be only invoked when initializing non-standard settings (as expected). E.g., SpaceGroup("Pmc2_1") does not invoke a warning, but SpaceGroup("P2_1ma") does.

However, I struggle to write a test for that:

    def test_full_symbol_warning(self):
        with pytest.warns(Warning,
                          match="Full symbol not available, falling back to short Hermann Mauguin symbol P2_1ma instead.") as warns:
            _ = SpaceGroup("P2_1ma")
        assert len(warns) == 1

This fails with Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted. Emitted warnings: []. and I don't understand why. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure you weren't testing against the PyPI release of pymatgen? your test passes for me. just pushed it to this PR. i expect it'll pass here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for figuring out this issue! a33cc3c

self.point_group = spg["schoenflies"].split("^")[0] # TODO map onto Hermann Mauguin notation?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's come to a decision here before merging this PR. what would be the benefits of mapping onto Hermann Mauguin notation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The information is the same, but in these cases we'd have different notations for space group (Hermann Mauguin) and point group (Schoenflies) within one object. Also, it might cause issues if one wants to compare the point groups for a larger dataset (and the same point group could be there in different representations).

In order to map it, we would either need a database or we could, in principle, extract the point group in Hermann Mauguin notation from the space group symbol (by removing the centering and translation parts of symmetry elements) - this would require additional testing, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there could be a PointGroup class method from_spacegroup() that does exactly this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds like a good idea! would you prefer to do that in a separate PR (or just open an issue for it)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, i'd prefer to just open an issue for it and add it in this PR - will do both tomorrow (:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the new class method and also added this info directly to the symm_ops.json database so it does not have to be called every time when initializing a SpaceGroup object. The changes to symm_ops.json and symm_data.json are documented in dev_scripts.update_space_group_data.py - was not sure if this is the correct place, though!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! this is great. it's super valuable to leave such a thorough paper trial for people to understand what happened later

self.int_number = spg["number"]
self.order = len(ops)
self._symmetry_ops = {*ops}
Expand Down Expand Up @@ -399,7 +412,7 @@ def check(param, ref, tolerance):
if crys_system == "hexagonal" or (
crys_system == "trigonal"
and (
self.symbol.endswith("H")
self.hexagonal
or self.int_number
in [143, 144, 145, 147, 149, 150, 151, 152, 153, 154, 156, 157, 158, 159, 162, 163, 164, 165]
)
Expand Down Expand Up @@ -531,7 +544,8 @@ def sg_symbol_from_int_number(int_number: int, hexagonal: bool = True) -> str:
hexagonal setting (default) or rhombohedral setting.

Returns:
str: Spacegroup symbol
str: Spacegroup symbol / Space group symbol + "H" if group is
rhombohedral and hexagonal=True
"""
syms = []
for n, v in SYMM_DATA["space_group_encoding"].items():
Expand Down
2 changes: 1 addition & 1 deletion pymatgen/symmetry/symm_ops.json
View file
Open in desktop

Large diffs are not rendered by default.

33 changes: 20 additions & 13 deletions tests/symmetry/test_groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,17 @@
ORDERED_SYMBOLS = (
"P1 P-1 P121 P12_11 C121 P1m1 P1c1 C1m1 C1c1 P12/m1 P12_1/m1 C12/m1 P12/c1 P12_1/c1 C12/c1 P222 P222_1"
" P2_12_12 P2_12_121 C222_1 C222 F222 I222 I2_12_121 Pmm2 Pmc2_1 Pcc2 Pma2 Pca2_1 Pnc2 Pmn2_1 Pba2 Pna2_1 Pnn2 "
"Cmm2 Cmc2_1 Ccc2 Amm2 Aem2 Ama2 Aea2 Fmm2 Fdd2 Imm2 Iba2 Ima2 Pmmm Pnnn1 Pccm Pban1 Pmma Pnna Pmna Pcca Pbam "
"Pccn Pbcm Pnnm Pmmn1 Pbcn Pbca Pnma Cmcm Cmce Cmmm Cccm Cmme Ccce1 Fmmm Fddd1 Immm Ibam Ibca Imma P4 P4_1 P4_2 "
"P4_3 I4 I4_1 P-4 I-4 P4/m P4_2/m P4/n1 P4_2/n I4/m I4_1/a P422 P42_12 P4_122 P4_12_12 P4_222 P4_22_12 P4_322 "
"Cmm2 Cmc2_1 Ccc2 Amm2 Aem2 Ama2 Aea2 Fmm2 Fdd2 Imm2 Iba2 Ima2 Pmmm Pnnn Pccm Pban Pmma Pnna Pmna Pcca Pbam "
"Pccn Pbcm Pnnm Pmmn Pbcn Pbca Pnma Cmcm Cmce Cmmm Cccm Cmme Ccce Fmmm Fddd Immm Ibam Ibca Imma P4 P4_1 P4_2 "
"P4_3 I4 I4_1 P-4 I-4 P4/m P4_2/m P4/n P4_2/n I4/m I4_1/a P422 P42_12 P4_122 P4_12_12 P4_222 P4_22_12 P4_322 "
"P4_32_12 I422 I4_122 P4mm P4bm P4_2cm P4_2nm P4cc P4nc P4_2mc P4_2bc I4mm I4cm I4_1md I4_1cd P-42m P-42c P-42_1m "
"P-42_1c P-4m2 P-4c2 P-4b2 P-4n2 I-4m2 I-4c2 I-42m I-42d P4/mmm P4/mcc P4/nbm1 P4/nnc1 P4/mbm P4/mnc P4/nmm1 "
"P4/ncc1 P4_2/mmc P4_2/mcm P4_2/nbc P4_2/nnm P4_2/mbc P4_2/mnm P4_2/nmc P4_2/ncm I4/mmm I4/mcm I4_1/amd I4_1/acd "
"P-42_1c P-4m2 P-4c2 P-4b2 P-4n2 I-4m2 I-4c2 I-42m I-42d P4/mmm P4/mcc P4/nbm P4/nnc P4/mbm P4/mnc P4/nmm "
"P4/ncc P4_2/mmc P4_2/mcm P4_2/nbc P4_2/nnm P4_2/mbc P4_2/mnm P4_2/nmc P4_2/ncm I4/mmm I4/mcm I4_1/amd I4_1/acd "
"P3 P3_1 P3_2 R3H P-3 R-3H P312 P321 P3_112 P3_121 P3_212 P3_221 R32H P3m1 P31m P3c1 P31c R3mH R3cH P-31m P-31c "
"P-3m1 P-3c1 R-3mH R-3cH P6 P6_1 P6_5 P6_2 P6_4 P6_3 P-6 P6/m P6_3/m P622 P6_122 P6_522 P6_222 P6_422 P6_322 "
"P6mm P6cc P6_3cm P6_3mc P-6m2 P-6c2 P-62m P-62c P6/mmm P6/mcc P6_3/mcm P6_3/mmc P23 F23 I23 P2_13 I2_13 Pm-3 "
"Pn-31 Fm-3 Fd-31 Im-3 Pa-3 Ia-3 P432 P4_232 F432 F4_132 I432 P4_332 P4_132 I4_132 P-43m F-43m I-43m P-43n F-43c "
"I-43d Pm-3m Pn-3n1 Pm-3n Pn-3m1 Fm-3m Fm-3c Fd-3m1 Fd-3c1 Im-3m Ia-3d"
"Pn-3 Fm-3 Fd-3 Im-3 Pa-3 Ia-3 P432 P4_232 F432 F4_132 I432 P4_332 P4_132 I4_132 P-43m F-43m I-43m P-43n F-43c "
"I-43d Pm-3m Pn-3n Pm-3n Pn-3m Fm-3m Fm-3c Fd-3m Fd-3c Im-3m Ia-3d"
).split()


Expand Down Expand Up @@ -150,6 +150,13 @@ def test_is_compatible(self):
assert sg.is_compatible(cubic)
assert sg.is_compatible(rhom)
assert not sg.is_compatible(hexagonal)
sg = SpaceGroup("R-3m", hexagonal=True)
assert not sg.is_compatible(cubic)
assert sg.is_compatible(hexagonal)
sg = SpaceGroup("R-3m", hexagonal=False)
assert sg.is_compatible(cubic)
assert sg.is_compatible(rhom)
assert not sg.is_compatible(hexagonal)
sg = SpaceGroup("Pnma")
assert sg.is_compatible(cubic)
assert sg.is_compatible(tet)
Expand Down Expand Up @@ -198,14 +205,9 @@ def test_subgroup_supergroup(self):
assert SpaceGroup("Pma2").is_subgroup(SpaceGroup("Pccm"))
assert not SpaceGroup.from_int_number(229).is_subgroup(SpaceGroup.from_int_number(230))

def test_hexagonal(self):
for num in (146, 148, 155, 160, 161, 166, 167):
sg = SpaceGroup.from_int_number(num, hexagonal=False)
assert not sg.symbol.endswith("H")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it as the representation ("H" as last character) is not part of the symbol attribute in any case now.
We could maybe still keep this test and replace the assertion with a test for the hexagonal attribute assert sg.hexagonal == False to check that this is given correctly to the SpaceGroup init.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replacing the assertion with a test for the hexagonal attribute sounds good


def test_string(self):
sg = SpaceGroup("R-3c")
assert sg.to_latex_string() == r"R$\overline{3}$cH"
assert sg.to_latex_string() == r"R$\overline{3}$c"
sg = SpaceGroup("P6/mmm")
assert sg.to_latex_string() == "P6/mmm"
sg = SpaceGroup("P4_1")
Expand All @@ -217,6 +219,11 @@ def test_repr(self):
symbol = ORDERED_SYMBOLS[num - 1]
assert repr(sg) in f"SpaceGroup({symbol=})"

def test_valid_symbol(self): # Added after issue #3845
for num in range(1, 231):
symbol = SpaceGroup.from_int_number(num).symbol
assert SpaceGroup(symbol).int_number == num
Comment on lines +245 to +248
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is definitely a good test to have and an argument in favor of the changes in this PR


def test_raises_on_bad_int_number(self):
for num in (-5, 0, 231, 1000):
with pytest.raises(ValueError, match=f"International number must be between 1 and 230, got {num}"):
Expand Down