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

Jump to bottom

is_subgroup() modifications in SpaceGroup and PointGroup #3941

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+97 −24
Open

is_subgroup() modifications in SpaceGroup and PointGroup #3941

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
338375d
Modified SpaceGroup.is_subgroup() following issue #3937, added tests.
kaueltzen Jul 19, 2024
eaf1a84
Added crystal_system attribute to PointGroup, modified is_subgroup wa…
kaueltzen Jul 19, 2024
466405e
Modified is_subgroup of PointGroup to raise NotImplementedError if gr…
kaueltzen Jul 19, 2024
13b5ccf
Enabled PointGroup init from different settings, added test for that,…
kaueltzen Jul 22, 2024
e1ec533
Removed bug in short Hermann Mauguin symbol of some trigonal groups i…
kaueltzen Jul 22, 2024
cbb1c88
Merge branch 'materialsproject:master' into group_subgroup
kaueltzen Jul 24, 2024
29a613d
Merge branch 'master' into group_subgroup
kaueltzen Sep 23, 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
58 changes: 53 additions & 5 deletions src/pymatgen/symmetry/groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,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 @@ -88,7 +91,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 •
edited
Loading

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 Down Expand Up @@ -125,6 +131,7 @@ def __init__(self, int_symbol: str) -> None:
]
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 @@ -166,6 +173,44 @@ 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
Expand Down Expand Up @@ -525,16 +570,19 @@ def is_subgroup(self, supergroup: SymmetryGroup) -> bool:
if not isinstance(supergroup, SpaceGroup):
return NotImplemented

if len(supergroup.symmetry_ops) < len(self.symmetry_ops):
return False
# if len(supergroup.symmetry_ops) < len(self.symmetry_ops): # Disabled after issue #3937 TODO constrain
# return False

groups = [{supergroup.int_number}]
all_groups = [supergroup.int_number]
max_subgroups = {int(k): v for k, v in SYMM_DATA["maximal_subgroups"].items()}
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
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 4 additions & 1 deletion tests/symmetry/test_groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ 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)

Expand Down Expand Up @@ -203,6 +204,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
Loading