only return unique point group operations

[mueslo]

Summary

• Fix duplicate point group operations returned in SpacegroupAnalyzer.get_point_group_operations

Retains original ordering, i.e. identity operation E always stays first.

Background: get_point_group_operations ultimately uses spglib.get_symmetry. However, this only returns unique tuples of (rotation, translation, time_reversal), as of spglib v2.0.2. So if you have n translations and m time reversals you will get your rotation symmop n*m times.

This is very annoying when e.g. filling the Brillouin zone with potentially tens of thousands of k-points from the irreducible wedge, and then getting hundreds of thousands of unnecessary duplicate k-points thanks to non-unique symmetry operations

Since SpacegroupAnalyzer._get_symmetry only uses and returns (rotation, translation), it should actually also be made unique, however since is_magnetic=True in spglib is supposedly being deprecated, this should solve itself.

Example

Input:

import numpy as np
from pymatgen.core.structure import Structure
from pymatgen.symmetry.analyzer import SpacegroupAnalyzer
struc = Structure.from_spacegroup(223, np.eye(3), ['Ni'], [[0., 0., 0.]])
pgops = SpacegroupAnalyzer(struc).get_point_group_operations(cartesian=True)

Output before fix

len(pgops) == 192
pgops ==
[Rot:
 [[1. 0. 0.]
  [0. 1. 0.]
  [0. 0. 1.]]
 tau
 [0. 0. 0.],
 Rot:
 [[1. 0. 0.]
  [0. 1. 0.]
  [0. 0. 1.]]
 tau
 [0. 0. 0.],
 Rot:
 [[-1.  0.  0.]
  [ 0. -1.  0.]
  [ 0.  0. -1.]]
 tau
 [0. 0. 0.],
 Rot:
 [[-1.  0.  0.]
  [ 0. -1.  0.]
  [ 0.  0. -1.]]
 tau
 [0. 0. 0.],
...]

Output after fix

len(pgops) == 48
pgops ==
[Rot:
 [[1. 0. 0.]
  [0. 1. 0.]
  [0. 0. 1.]]
 tau
 [0. 0. 0.],
 Rot:
 [[-1.  0.  0.]
  [ 0. -1.  0.]
  [ 0.  0. -1.]]
 tau
 [0. 0. 0.],
...]

Speed

I found rot.tobytes() seems to be the fastest hashable option for the set. Overall, also thanks to the removed 3x3 matrix inversion and lookup instead, and the skipped SymmOp __init__s, the new function is >10% faster for both low and high counts of symmetry operations.

>>> %timeit sa.get_point_group_operations(cartesian=True)
>>> %timeit sa.get_point_group_operations_new(cartesian=True)
<<< 8.29 ms ± 48.9 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
<<< 6.76 ms ± 23.4 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Time taken drops further to ~5ms if not going via self._get_symmetry() which has unnecessary translation vector logic, but directly calling spglib.get_symmetry(), at the cost of duplicate code. So I did not include it in this PR.

Checklist

Work-in-progress pull requests are encouraged, but please put [WIP] in the pull request title.

Before a pull request can be merged, the following items must be checked:

• Tests have been added for any new functionality or bug fixes.
• All linting and tests pass.

[janosh]

Thanks @mueslo. For future reference, could you add a failing example as JSON or CIF to the PR description using <details>?

<details><summary>example</summary>
<p>

hi there!

</p>
</details> 

[mueslo]

@janosh I added some details and a minimal working example. Could you please trigger the tests?

I also note that by default SpacegroupAnalyzer._get_symmetry also returns non-unique operations due the default is_magnetic being True in spglib.get_symmetry. However, passing is_magnetic=False is not a good option since the parameter is deprecated, so might give errors in the future when it is removed. For the time being I could also make SpacegroupAnalyzer._get_symmetry give only unique (rotations, translations) as part of this PR, if you think it might be a good idea.

[janosh]

@mueslo Very sorry for the long delay! I somehow wasn't pinged about this (or missed it). Thanks for the additional details. Tests running now.

For the time being I could also make SpacegroupAnalyzer._get_symmetry give only unique (rotations, translations) as part of this PR, if you think it might be a good idea.

I'll defer to @shyuep on this decision who will be better informed to gauge the downstream implications. Incidentally, he just bumped spglib to v2.0.2 in 3d33aca.

[janosh]

@mueslo If this is ready to go, best remove "WIP" from PR title as that makes it more likely to receive attention.

[janosh]

@mueslo The linter will be fixed if you pull in master.

[janosh]

For the time being I could also make SpacegroupAnalyzer._get_symmetry give only unique (rotations, translations) as part of this PR, if you think it might be a good idea.

I'll defer to @shyuep on this decision who will be better informed to gauge the downstream implications. Incidentally, he just bumped spglib to v2.0.2 in 3d33aca.

I'll merge this for now. Feel free to open a new PR for SpacegroupAnalyzer._get_symmetry.