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

is_subgroup() modifications in SpaceGroup and PointGroup #3941

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
7 changes: 7 additions & 0 deletions dev_scripts/update_spacegroup_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ def remove_identity_from_full_hermann_mauguin(symbol: str) -> str:
if symbol in ("P 1", "C 1", "P 1 "):
return symbol
blickrichtungen = symbol.split(" ")
if blickrichtungen[1].startswith("3"):
return symbol
blickrichtungen_new = []
for br in blickrichtungen:
if br != "1":
Expand All @@ -76,6 +78,11 @@ def remove_identity_from_full_hermann_mauguin(symbol: str) -> str:

SYMM_DATA["space_group_encoding"] = new_symm_data

for sg_symbol in SYMM_DATA["space_group_encoding"]:
SYMM_DATA["space_group_encoding"][sg_symbol]["point_group"] = PointGroup.from_space_group(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I would like to have some more opinions on this: With the current changes, all "point_group" entries in symm_data.json and symm_ops.json are possible symbols of PointGroup. This means that the setting of a SpaceGroup object and its point_group attribute may be different, for example, "P-4m2" and "-42m" (as there is no "-4m2" setting of this point group in symm_data.json currently.
Therefore, SpaceGroup objects of the same point group always have the same point_group attribute.
Also, this enables quick comparisons like https://github.com/kaueltzen/pymatgen/blob/e1ec53387a22012936afbf693fcd623a405db690/src/pymatgen/symmetry/groups.py#L584 but of course one could also agree on a "non-standard" point_group attribute in agreement with the space group setting and adapt respective code snippets
if len(supergroup.symmetry_ops) < len(self.symmetry_ops) and PointGroup(supergroup.point_group).symbol != PointGroup(self.point_group).symbol:

Copy link
Member

Choose a reason for hiding this comment

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

is this still waiting on external input? looks like this PR went stale so maybe need to ping someone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @janosh , originally, i was hoping for input from you, @mkhorton or @shyuep . However, if there are no strong opinions on this, these changes can be merged.

sg_symbol=sg_symbol
).symbol

for spg in SYMM_OPS:
if "(" in spg["hermann_mauguin"]:
spg["hermann_mauguin"] = spg["hermann_mauguin"].split("(")[0]
Expand Down
101 changes: 80 additions & 21 deletions src/pymatgen/symmetry/groups.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,24 @@

SYMM_DATA = loadfn(os.path.join(os.path.dirname(__file__), "symm_data.json"))

PG_ABBREV_MAP = {
"2/m2/m2/m": "mmm",
"4/m2/m2/m": "4/mmm",
"-32/m": "-3m",
"6/m2/m2/m": "6/mmm",
"2/m-3": "m-3",
"4/m-32/m": "m-3m",
}
PG_SETTINGS_MAP = { # only one setting per crystal class in SYMM_DATA database available right now
"m2m": "mm2",
"2mm": "mm2",
"-4m2": "-42m",
"-62m": "-6m2",
"312": "32",
"31m": "3m",
"-31m": "-3m",
}


class SymmetryGroup(Sequence, Stringify, ABC):
"""Abstract class representing a symmetry group."""
Expand Down Expand Up @@ -78,7 +96,10 @@ def is_subgroup(self, supergroup: SymmetryGroup) -> bool:
Returns:
bool: True if this group is a subgroup of the supplied group.
"""
warnings.warn("This is not fully functional. Only trivial subsets are tested right now. ")
warnings.warn(
"This is not fully functional. Only trivial subsets are tested right now. "
"This will not work if the crystallographic directions of the two groups are different."
)
return set(self.symmetry_ops).issubset(supergroup.symmetry_ops)

def is_supergroup(self, subgroup: SymmetryGroup) -> bool:
Expand All @@ -90,7 +111,10 @@ def is_supergroup(self, subgroup: SymmetryGroup) -> bool:
Returns:
bool: True if this group is a supergroup of the supplied group.
"""
warnings.warn("This is not fully functional. Only trivial subsets are tested right now. ")
warnings.warn(
"This is not fully functional. Only trivial subsets are tested right now. "
Copy link
Member

Choose a reason for hiding this comment

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

typo: Only trivial (subsets->supergroup)...

maybe extract these duplicate messages into a trivial_group_warning format string with a which_set parameter that you fill with subgroup/supergroup in def is_(sub|super)group

Copy link
Contributor Author

@kaueltzen kaueltzen Aug 2, 2024

Choose a reason for hiding this comment

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

Hi, I'm not sure what you mean @janosh . A function like this?

def trivial_group_warning(subgroup: str, supergroup: str) -> str:
    return (
        f"This is not fully functional. Only trivial subsets are tested right now."
        f"This will not work if the crystallographic directions of subgroup "
        f"{subgroup} and supergroup {supergroup} are different."
    )

that is called in is_(sub|super)group?

warnings.warn(trivial_group_warning(subgroup=self.symbol, supergroup=supergroup.symbol))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @janosh can you maybe elaborate on your comment from Aug 2? Thanks!

"This will not work if the crystallographic directions of the two groups are different."
)
return set(subgroup.symmetry_ops).issubset(self.symmetry_ops)

def to_latex_string(self) -> str:
Expand All @@ -117,16 +141,23 @@ def __init__(self, int_symbol: str) -> None:
Please note that only the 32 crystal classes are supported right now.

Args:
int_symbol (str): International or Hermann-Mauguin Symbol.
int_symbol (str): International or Hermann-Mauguin Symbol. Please note that the PointGroup object
may have a different setting than specified in the int_symbol as only one setting per point group
is available right now.
"""
from pymatgen.core.operations import SymmOp

int_symbol = int_symbol.replace(" ", "")
int_symbol = PG_ABBREV_MAP.get(int_symbol, int_symbol)
int_symbol = PG_SETTINGS_MAP.get(int_symbol, int_symbol)

self.symbol = int_symbol
self.generators = [
SYMM_DATA["generator_matrices"][enc] for enc in SYMM_DATA["point_group_encoding"][int_symbol]
]
self._symmetry_ops = {SymmOp.from_rotation_and_translation(m) for m in self._generate_full_symmetry_ops()}
self.order = len(self._symmetry_ops)
self.crystal_system = SYMM_DATA["point_group_crystal_system_map"][int_symbol]

@property
def symmetry_ops(self) -> set[SymmOp]:
Expand Down Expand Up @@ -168,10 +199,49 @@ def get_orbit(self, p: ArrayLike, tol: float = 1e-5) -> list[np.ndarray]:
orbit.append(pp)
return orbit

def is_subgroup(self, supergroup: PointGroup) -> bool:
kaueltzen marked this conversation as resolved.
Show resolved Hide resolved
"""True if this group is a subgroup of the supplied group.
Modification of SymmetryGroup method with a few more constraints.

Args:
supergroup (pointGroup): Supergroup to test.

Returns:
bool: True if this group is a subgroup of the supplied group.
"""
possible_but_direction_differences = (
["trigonal", "cubic"],
["monoclinic", "tetragonal"],
["monoclinic", "hexagonal"],
["monoclinic", "trigonal"],
["orthorhombic", "hexagonal"],
["orthorhombic", "tetragonal"],
)
if [self.crystal_system, supergroup.crystal_system] in possible_but_direction_differences:
raise NotImplementedError
warnings.warn(
"This is not fully functional. Only trivial subsets are tested right now. "
"This will not work if the crystallographic directions of the two groups are different."
)
return set(self.symmetry_ops).issubset(supergroup.symmetry_ops)

def is_supergroup(self, subgroup: PointGroup) -> bool:
"""True if this group is a subgroup of the supplied group.
Modification of SymmetryGroup method with a few more constraints.

Args:
subgroup (PointGroup): Subgroup to test.

Returns:
bool: True if this group is a supergroup of the supplied group.
"""
return subgroup.is_subgroup(self)

@classmethod
def from_space_group(cls, sg_symbol: str) -> PointGroup:
"""Instantiate one of the 32 crystal classes from a space group symbol in
Hermann Mauguin notation (int symbol or full symbol).
Please note that the axes of space group and crystal class may be different.

Args:
sg_symbol: space group symbol in Hermann Mauguin notation.
Expand All @@ -182,20 +252,6 @@ def from_space_group(cls, sg_symbol: str) -> PointGroup:
Returns:
crystal class in Hermann-Mauguin notation.
"""
abbrev_map = {
"2/m2/m2/m": "mmm",
"4/m2/m2/m": "4/mmm",
"-32/m": "-3m",
"6/m2/m2/m": "6/mmm",
"2/m-3": "m-3",
"4/m-32/m": "m-3m",
}
non_standard_map = {
"m2m": "mm2",
"2mm": "mm2",
"-4m2": "-42m", # technically not non-standard
"-62m": "-6m2", # technically not non-standard
}
symbol = re.sub(r" ", "", sg_symbol)

symm_ops = loadfn(os.path.join(os.path.dirname(__file__), "symm_ops.json")) # get short symbol if possible
Expand All @@ -211,8 +267,8 @@ def from_space_group(cls, sg_symbol: str) -> PointGroup:
symbol = symbol[1:] # Remove centering
symbol = symbol.translate(str.maketrans("abcden", "mmmmmm")) # Remove translation from glide planes
symbol = re.sub(r"_.", "", symbol) # Remove translation from screw axes
symbol = abbrev_map.get(symbol, symbol)
symbol = non_standard_map.get(symbol, symbol)
symbol = PG_ABBREV_MAP.get(symbol, symbol)
symbol = PG_SETTINGS_MAP.get(symbol, symbol)

if symbol not in SYMM_DATA["point_group_encoding"]:
raise ValueError(f"Could not create a valid crystal class ({symbol}) from {sg_symbol=}")
Expand Down Expand Up @@ -530,7 +586,7 @@ def is_subgroup(self, supergroup: SymmetryGroup) -> bool:
if not isinstance(supergroup, SpaceGroup):
return NotImplemented

if len(supergroup.symmetry_ops) < len(self.symmetry_ops):
if len(supergroup.symmetry_ops) < len(self.symmetry_ops) and supergroup.point_group != self.point_group:
return False

groups = [{supergroup.int_number}]
Expand All @@ -539,7 +595,10 @@ def is_subgroup(self, supergroup: SymmetryGroup) -> bool:
while True:
new_sub_groups = set()
for i in groups[-1]:
new_sub_groups.update([j for j in max_subgroups[i] if j not in all_groups])
if len(groups) == 1:
new_sub_groups.update(list(max_subgroups[i]))
else:
new_sub_groups.update([j for j in max_subgroups[i] if j not in all_groups])
if self.int_number in new_sub_groups:
return True

Expand Down
2 changes: 1 addition & 1 deletion src/pymatgen/symmetry/symm_data.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/pymatgen/symmetry/symm_ops.json

Large diffs are not rendered by default.

9 changes: 8 additions & 1 deletion tests/symmetry/test_groups.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,16 @@ def test_is_sub_super_group(self):
pg_m3m = PointGroup("m-3m")
pg_6mmm = PointGroup("6/mmm")
pg_3m = PointGroup("-3m")
# TODO: Fix the test below.
# TODO: Fix the tests below.
# assert pg3m.is_subgroup(pgm3m)
# assert PointGroup("32").is_subgroup(PointGroup("-6m2"))
assert pg_3m.is_subgroup(pg_6mmm)
assert not pg_m3m.is_supergroup(pg_6mmm)

def test_init_different_settings_and_full_notation(self):
PointGroup("2/m 2/m 2/m")
PointGroup("31m")

def test_from_space_group(self):
assert PointGroup.from_space_group("P 2_1/n2_1/m2_1/a").symbol == "mmm"
assert PointGroup.from_space_group("F d d d").symbol == "mmm"
Expand Down Expand Up @@ -204,6 +209,8 @@ def test_other_settings(self):
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))
assert SpaceGroup("P3").is_subgroup(SpaceGroup("P3")) # Added after #3937
assert SpaceGroup("Fm-3m").is_subgroup(SpaceGroup("Pm-3m")) # Added after #3937

def test_hexagonal(self):
for num in (146, 148, 155, 160, 161, 166, 167):
Expand Down