-
Notifications
You must be signed in to change notification settings - Fork 878
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
Changes from 6 commits
338375d
eaf1a84
466405e
13b5ccf
e1ec533
cbb1c88
29a613d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,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.""" | ||
|
@@ -76,7 +94,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: | ||
|
@@ -88,7 +109,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. " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: maybe extract these duplicate messages into a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
that is called in
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
@@ -115,16 +139,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]: | ||
|
@@ -166,10 +197,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. | ||
|
@@ -179,20 +249,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 | ||
|
@@ -206,8 +262,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) | ||
|
||
assert ( | ||
symbol in SYMM_DATA["point_group_encoding"] | ||
|
@@ -525,7 +581,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}] | ||
|
@@ -534,7 +590,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 | ||
|
||
|
Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
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 insymm_data.json
andsymm_ops.json
are possible symbols ofPointGroup
. This means that the setting of aSpaceGroup
object and itspoint_group
attribute may be different, for example, "P-4m2" and "-42m" (as there is no "-4m2" setting of this point group insymm_data.json
currently.Therefore,
SpaceGroup
objects of the same point group always have the samepoint_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 snippetsif len(supergroup.symmetry_ops) < len(self.symmetry_ops) and PointGroup(supergroup.point_group).symbol != PointGroup(self.point_group).symbol:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.