SpaceGroup changes #3859

kaueltzen · 2024-06-04T12:13:07Z

Summary

Includes the following changes in the symmetry module:

The SpaceGroup symbol attribute now refers to its Hermann-Mauguin symbol with underscore notation of screw axes.
The setting of the 7 rhombohedral space group types is now stored in an additional hexagonal bool attribute which can be set explicitly or by appending the setting to the int_symbol string parameter with the setting appended as before. The latter overrides the former for backwards compatibility.
The SpaceGroup point_group attribute now always refers to the point group in Hermann Mauguin notation.
A class method from_space_group was added to PointGroup which initializes the 32 crystal classes from a SpaceGroup string in Hermann-Mauguin notation.
Changes of the space group databases symm_data.json and symm_ops.json were made which are documented in dev_scripts/update_spacegroup_data.py .
If the full symbol is not available (for non-standard settings), a warning is now issued.
Tests for the abovementioned points were added
Notes on how to convert the old to the new symbol were added to docs/compatibility.md

… corrected Schoenflies point group attribute, changed handling of rhombohedral space group type settings by adding a SpaceGroup hexagonal bool attribute, modified and added tests.

janosh

thanks @kaueltzen for submitting this fix! 👍 i left 3 questions

janosh · 2024-06-04T13:29:56Z

pymatgen/symmetry/groups.py

                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?


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

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?)

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?

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

Thanks for figuring out this issue! a33cc3c

janosh · 2024-06-04T13:30:38Z

pymatgen/symmetry/groups.py

-                    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?
+                    self.point_group = spg["schoenflies"].split("^")[0]  # TODO map onto Hermann Mauguin notation?


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

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.

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

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

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

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!

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

janosh · 2024-06-04T13:31:50Z

tests/symmetry/test_groups.py

+    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


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

janosh · 2024-06-04T13:32:39Z

tests/symmetry/test_groups.py

-    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")


why remove this test?

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.

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

mkhorton · 2024-06-04T22:46:34Z

Many thanks for this @kaueltzen!

In case it is helpful in future, adding a note that the maggroups module in pymatgen uses a different dataset (from ISO-MAG, used with permission). It may or may not be helpful to cross-check information, since it's a superset of the crystallographic groups.

Important note for this PR: this technically counts as a breaking change. I can imagine cases whereby users are generating and storing space group strings in a database, for example, with the previous logic. Therefore, can you add a note to docs/compatibility.md noting the change, and how affected users can replace their previous symbol with the new symbol?

JaGeo · 2024-06-05T03:56:28Z

Important note for this PR: this technically counts as a breaking change. I can imagine cases whereby users are generating and storing space group strings in a database, for example, with the previous logic. Therefore, can you add a note to docs/compatibility.md noting the change, and how affected users can replace their previous symbol with the new symbol?

Thanks @mkhorton ! We know it is breaking. We had asked in the issue how to handle this. Thanks for pointing out how to do it in the best way! Very helpful.

mkhorton · 2024-06-05T04:01:28Z

No problem, apologies I’d missed the issue comment.

kaueltzen · 2024-06-05T08:42:57Z

Many thanks for this @kaueltzen!

In case it is helpful in future, adding a note that the maggroups module in pymatgen uses a different dataset (from ISO-MAG, used with permission). It may or may not be helpful to cross-check information, since it's a superset of the crystallographic groups.

Important note for this PR: this technically counts as a breaking change. I can imagine cases whereby users are generating and storing space group strings in a database, for example, with the previous logic. Therefore, can you add a note to docs/compatibility.md noting the change, and how affected users can replace their previous symbol with the new symbol?

Thanks @mkhorton for pointing that out! Will add a note.

…om int number.

…ility.md

kaueltzen · 2024-06-05T11:09:56Z

Hey, while working on the new PointGroup class method from_space_group(), I found another bug in SpaceGroup referenced here: #3862 which is also important for this PR.
What are your thoughts on that? @JaGeo @janosh @mkhorton

kaueltzen · 2024-06-05T16:49:11Z

Another minor issue: the space group types I2_12_12_1 and P2_12_12_1 are mapped onto I/P2_12_121 in SpaceGroup.full_sg_mapping , the wrong key is also there in SpaceGroup.sg_encoding . Will fix that as well.

…odified symm_ops.json and symm_data.json (documented in dev_scripts/update_space_group_data.py) to have same notation, added point group and short Hermann Mauguin symbol to symm_ops, fixed some typos, fixed rhombohedral space group type orbit issue.

Signed-off-by: Katharina Ueltzen <94910364+kaueltzen@users.noreply.github.com>

…mpatible().

…on-underscore space group notations.

… point_group attribute setting.

…entity blickrichtungen and missed underscores, added warning to SpaceGroup init if full symbol is not available (for non-standard settings), added tests.

…und to annoying test pollution from side effects to std lib warnings registry

…12_12_1 and I2_12_12_1.

kaueltzen · 2024-06-19T08:55:34Z

Hey @mkhorton @shyuep this is ready to be reviewed.

JaGeo · 2024-06-24T14:46:06Z

As far as I see it, all review requests have been addressed by @kaueltzen . Would be great if this could be merged if the changes are fine.

JaGeo · 2024-06-27T15:42:25Z

Oh, now there are some conflicts 🫠

shyuep · 2024-06-27T16:06:23Z

It is just a switch to a src based layout. Once you move the files, the conflicts should be simple to resolve.

Resolve merge conflicts.

kaueltzen · 2024-07-05T13:55:27Z

It is just a switch to a src based layout. Once you move the files, the conflicts should be simple to resolve.

Hey @shyuep @mkhorton the merge conflicts have been resolved. Could you please review this?

JaGeo · 2024-07-14T15:11:39Z

@shyuep is there anything that still needs to be done here? We would be very happy if this could be merged and fixed.

shyuep · 2024-07-14T19:30:35Z

Merged. Thanks,

JaGeo · 2024-07-14T19:35:40Z

@shyuep thank you!

DanielYang59 · 2024-07-15T07:35:42Z

Looks like this PR breaks one of the unit tests in windows?

 _______________ TestSpacegroupAnalyzer.test_get_crystal_system ________________

self = <symmetry.test_analyzer.TestSpacegroupAnalyzer testMethod=test_get_crystal_system>

    def test_get_crystal_system(self):
        crystal_system = self.sg.get_crystal_system()
        assert crystal_system == "orthorhombic"
        assert self.disordered_sg.get_crystal_system() == "tetragonal"
    
        orig_spg = self.sg._space_group_data["number"]
>       self.sg._space_group_data["number"] = 0
E       TypeError: 'SpglibDataset' object does not support item assignment

Feel free to ping me if there's anything I could help :)

JaGeo · 2024-07-15T07:49:28Z

@kaueltzen could you maybe take a look?

kaueltzen · 2024-07-15T09:40:45Z

Hi @DanielYang59 i cannot see where this might be due to the SpaceGroup changes. it seems like it is due to the latest spglib release? https://spglib.readthedocs.io/en/stable/releases.html#v2-5-0-9-jul-2024

The output data is not longer in dict format but in classes like SpglibDataset. The dict-like interface is planned to be removed, so there will be a lot of necessary changes in pymatgen. would modifiying the version of spglib to <2.5.0 in pyproject.toml be ok for you for now?

DanielYang59 · 2024-07-15T11:29:21Z

@kaueltzen Apologize, I didn't look very closely into the failure (just noticed unit tests begin to fail after this PR, and this is related to spacegroup). Sorry for misleading you.

I could confirm using spglig lower than 2.5.0 fixed this meanwhile :)

kaueltzen · 2024-07-15T12:02:50Z

I could confirm using spglig lower than 2.5.0 fixed this meanwhile :)

Great 👍 and thanks for looking into this!

…ntroduced by myself in materialsproject#3859

* Modified SpaceGroup.is_subgroup() following issue #3937, added tests. * Added crystal_system attribute to PointGroup, modified is_subgroup warning of SymmetryGroup. * Modified is_subgroup of PointGroup to raise NotImplementedError if group subgroup relationship would be possible (fr. crystal system), but cannot be checked currently due to crystallographic direction issue. * Enabled PointGroup init from different settings, added test for that, added missing setting differences (trigonal groups), re-enabled SpaceGroup.is_subgroup shortcut with constraint of non-klassengleiche group subgroup relationship * Removed bug in short Hermann Mauguin symbol of some trigonal groups introduced by myself in #3859 --------- Signed-off-by: Katharina Ueltzen <94910364+kaueltzen@users.noreply.github.com>

kaueltzen and others added 3 commits June 4, 2024 13:21

Replaced SpaceGroup symbol attribute with its Hermann-Mauguin symbol,…

ebc35f0

… corrected Schoenflies point group attribute, changed handling of rhombohedral space group type settings by adding a SpaceGroup hexagonal bool attribute, modified and added tests.

Removed crystal class key from symm_ops.json.

fe7363d

Merge branch 'materialsproject:master' into space_group

ebe1246

kaueltzen requested review from shyuep, janosh and mkhorton as code owners June 4, 2024 12:13

janosh suggested changes Jun 4, 2024

View reviewed changes

Test for correct setting of hexagonal attribute when instantiating fr…

561dfef

…om int number.

kaueltzen mentioned this pull request Jun 5, 2024

Add from_space_group() class method to PointGroup #3861

Closed

Noted change and replacement option for SpaceGroup symbol in compatib…

25f90e7

…ility.md

kaueltzen mentioned this pull request Jun 5, 2024

SpaceGroup object init and string representation issues for space group types with screw axes in non-standard settings #3862

Closed

janosh added fix Bug fix PRs breaking Breaking change symmetry Space groups and the like labels Jun 5, 2024

kaueltzen and others added 10 commits June 6, 2024 13:10

Added from_space_group class method to PointGroup, added tests

d0c50ba

Merge branch 'master' into space_group

0eea0bd

Signed-off-by: Katharina Ueltzen <94910364+kaueltzen@users.noreply.github.com>

Updated core/test_surface.py to assign lattice as in SpaceGroup is_co…

c008de9

…mpatible().

Modified databases and SpaceGroup init to ensure compatibility with n…

0b1dd3c

…on-underscore space group notations.

Added tests for issue materialsproject#3862, modified full_symbol and…

39d3683

… point_group attribute setting.

Modified PointGroup.from_space_group() to also handle symbols with id…

b6d6c25

…entity blickrichtungen and missed underscores, added warning to SpaceGroup init if full symbol is not available (for non-standard settings), added tests.

Added test for warning if SpaceGroup.full_symbol is not available.

6a089c7

Removed warning test.

ccc4cc8

Merge branch 'materialsproject:master' into space_group

828cf5d

janosh and others added 5 commits June 10, 2024 07:09

add test_full_symbol_warning

1d744b9

add author + date to dev_scripts/update_spacegroup_data.py

15acd36

typos

6439fed

warning occurs only once, move test_full_symbol_warning up as workaro…

a33cc3c

…und to annoying test pollution from side effects to std lib warnings registry

Updated compatibility.md to also handle old symbol replacement of P2_…

81f76f4

…12_12_1 and I2_12_12_1.

kaueltzen requested review from JaGeo and Andrew-S-Rosen as code owners July 4, 2024 10:06

Merge branch 'master' into space_group

26e7728

Resolve merge conflicts.

kaueltzen force-pushed the space_group branch from 1b8446e to 26e7728 Compare July 5, 2024 13:20

pre-commit-ci bot and others added 2 commits July 5, 2024 13:21

pre-commit auto-fixes

56416f2

Updated dev script path to new src layout.

21777c7

Merge branch 'master' into space_group

adcff10

shyuep merged commit c2c5ea6 into materialsproject:master Jul 14, 2024
31 of 33 checks passed

kaueltzen mentioned this pull request Jul 16, 2024

Adapt pymatgen.symmetry.analyzer to new spglib (>=2.5.0) output data format #3922

Closed

kaueltzen added a commit to kaueltzen/pymatgen that referenced this pull request Jul 22, 2024

Removed bug in short Hermann Mauguin symbol of some trigonal groups i…

e1ec533

…ntroduced by myself in materialsproject#3859

kaueltzen mentioned this pull request Jul 22, 2024

is_subgroup() modifications in SpaceGroup and PointGroup #3941

Merged

SpaceGroup changes #3859

SpaceGroup changes #3859

Conversation

kaueltzen commented Jun 4, 2024 • edited Loading

Summary

janosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhorton commented Jun 4, 2024

JaGeo commented Jun 5, 2024

mkhorton commented Jun 5, 2024

kaueltzen commented Jun 5, 2024

kaueltzen commented Jun 5, 2024 • edited Loading

kaueltzen commented Jun 5, 2024 • edited Loading

kaueltzen commented Jun 19, 2024

JaGeo commented Jun 24, 2024

JaGeo commented Jun 27, 2024

shyuep commented Jun 27, 2024

kaueltzen commented Jul 5, 2024

JaGeo commented Jul 14, 2024

shyuep commented Jul 14, 2024

JaGeo commented Jul 14, 2024

DanielYang59 commented Jul 15, 2024

JaGeo commented Jul 15, 2024

kaueltzen commented Jul 15, 2024 • edited Loading

DanielYang59 commented Jul 15, 2024 • edited Loading

kaueltzen commented Jul 15, 2024

kaueltzen commented Jun 4, 2024 •

edited

Loading

kaueltzen commented Jun 5, 2024 •

edited

Loading

kaueltzen commented Jun 5, 2024 •

edited

Loading

kaueltzen commented Jul 15, 2024 •

edited

Loading

DanielYang59 commented Jul 15, 2024 •

edited

Loading