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

Fix equivalent atoms and add tests #219

Merged
merged 4 commits into from
Jul 18, 2024
Merged

Fix equivalent atoms and add tests #219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
892f374
Fix equivalent atoms and add tests
jan-janssen Jul 17, 2024
15c5e53
Format black
pyiron-runner Jul 17, 2024
272abe2
Skip spglib tests when spglib is not available
jan-janssen Jul 17, 2024
3d6d312
Add another test
jan-janssen Jul 17, 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
4 changes: 3 additions & 1 deletion structuretoolkit/analyse/phonopy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,7 @@ def get_equivalent_atoms(
positions = np.reshape(np.array(positions), (natom, 3))
cell = np.reshape(np.array(cell), (3, 3))
unitcell = PhonopyAtoms(symbols=types, cell=cell, scaled_positions=positions)
ops = spg.get_symmetry(unitcell, symprec=symprec, angle_tolerance=angle_tolerance)
ops = spg.get_symmetry(
cell=unitcell.totuple(), symprec=symprec, angle_tolerance=angle_tolerance
)
Comment on lines +44 to +46
Copy link

Choose a reason for hiding this comment

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

Consider adding documentation for the use of totuple().

Adding a comment or updating the documentation to explain why totuple() is used instead of the direct unitcell object could help maintain clarity and assist future maintainers or users of the code.

+    # Convert unitcell to tuple to ensure compatibility with spglib API requirements
     ops = spg.get_symmetry(
         cell=unitcell.totuple(), symprec=symprec, angle_tolerance=angle_tolerance
     )
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ops = spg.get_symmetry(
cell=unitcell.totuple(), symprec=symprec, angle_tolerance=angle_tolerance
)
# Convert unitcell to tuple to ensure compatibility with spglib API requirements
ops = spg.get_symmetry(
cell=unitcell.totuple(), symprec=symprec, angle_tolerance=angle_tolerance
)

return ops["equivalent_atoms"]
31 changes: 31 additions & 0 deletions tests/test_analyse_phonopy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import unittest
from ase.build import bulk
import numpy as np
from structuretoolkit.analyse.phonopy import get_equivalent_atoms
from structuretoolkit.common.phonopy import atoms_to_phonopy, phonopy_to_atoms


try:
import spglib

spglib_not_available = False
except ImportError:
spglib_not_available = True


@unittest.skipIf(
spglib_not_available, "spglib is not installed, so the spglib tests are skipped."
)
class TestPhonopyAtoms(unittest.TestCase):
def test_get_equivalent_atoms(self):
equivalent_atoms = get_equivalent_atoms(
structure=bulk("Al", cubic=True), symprec=1e-5, angle_tolerance=-1.0
)
self.assertTrue(all(equivalent_atoms == [0, 0, 0, 0]))
Comment on lines +20 to +24
Copy link

Choose a reason for hiding this comment

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

Expand test coverage for get_equivalent_atoms.

The current test checks a specific scenario with a hard-coded expected result. Consider adding more test cases to cover different scenarios and edge cases, such as non-cubic structures or invalid inputs.


def test_convert(self):
structure = bulk("Al", cubic=True)
structure_converted = phonopy_to_atoms(atoms_to_phonopy(structure))
self.assertTrue(np.all(structure.symbols == structure_converted.symbols))
self.assertTrue(np.all(structure.positions == structure_converted.positions))
self.assertTrue(np.all(structure.cell == structure_converted.cell))