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: 781 pymatgen structure bug #782

Merged
merged 5 commits into from
Jan 17, 2025
Merged

fix: 781 pymatgen structure bug #782

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9ad562b
fix: 781 pymatgen structure bug
Jan 17, 2025
d7f54f2
add missing data
Jan 17, 2025
09f97fc
Merge branch 'devel' into fix-pmg
wanghan-iapcm Jan 17, 2025
329ca1e
fix nopbc
Jan 17, 2025
90a489b
Merge remote-tracking branch 'origin/fix-pmg' into fix-pmg
Jan 17, 2025
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: 1 addition & 3 deletions dpdata/plugins/pymatgen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ def to_system(self, data, **kwargs):
except ModuleNotFoundError as e:
raise ImportError("No module pymatgen.Structure") from e

species = []
for name, numb in zip(data["atom_names"], data["atom_numbs"]):
species.extend([name] * numb)
species = [data["atom_names"][tt] for tt in data["atom_types"]]
for ii in range(data["coords"].shape[0]):
structure = Structure(
data["cells"][ii],
Expand Down
11 changes: 10 additions & 1 deletion dpdata/pymatgen/structure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,27 @@


def from_system_data(structure) -> dict:
symbols = [site.species_string for site in structure]
"""Convert one pymatgen structure to dpdata's datadict."""
symbols = [ii.specie.symbol for ii in structure]
atom_names = list(structure.symbol_set)
atom_numbs = [symbols.count(symbol) for symbol in atom_names]
atom_types = np.array([atom_names.index(symbol) for symbol in symbols]).astype(int)
coords = structure.cart_coords
cells = structure.lattice.matrix
if all(structure.pbc):
pbc = True
elif not any(structure.pbc):
pbc = False

Check warning on line 17 in dpdata/pymatgen/structure.py

View check run for this annotation

Codecov / codecov/patch

dpdata/pymatgen/structure.py#L16-L17 

Added lines #L16 - L17 were not covered by tests
else:
raise ValueError(f"Partial pbc condition {structure.pbc} is not supported")

Check warning on line 19 in dpdata/pymatgen/structure.py

View check run for this annotation

Codecov / codecov/patch

dpdata/pymatgen/structure.py#L19 

Added line #L19 was not covered by tests
Comment on lines +14 to +19
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Add test coverage for PBC validation in pymatgen structure conversion.

The review comment is accurate. While there are tests for fully periodic and non-periodic cases in the codebase, there is no test coverage for:

  • Mixed PBC conditions (e.g. [True, False, True])
  • ValueError being raised for partial PBC cases

Please add test cases in tests/test_pymatgen_structure.py to verify this validation logic.

🔗 Analysis chain

Add test coverage for PBC validation.

The new PBC validation logic is a good addition, but static analysis indicates missing test coverage for the partial PBC case.

Let's verify the test coverage:

Consider adding test cases for:

  1. Fully periodic structure (all True)
  2. Non-periodic structure (all False)
  3. Partial periodic structure (mixed True/False) to verify error handling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test cases covering PBC validation
rg -l "pbc.*True|pbc.*False" tests/

Length of output: 261

Script:

#!/bin/bash
# Find pymatgen structure-specific test files
fd test.*pymatgen tests/

# Search for actual test cases with PBC validation in pymatgen tests
echo "=== Test content ==="
rg -A 5 -B 5 "def test.*pbc|assert.*pbc" tests/ --type py

Length of output: 4417

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 16-17: dpdata/pymatgen/structure.py#L16-L17
Added lines #L16 - L17 were not covered by tests

[warning] 19-19: dpdata/pymatgen/structure.py#L19
Added line #L19 was not covered by tests


info_dict = {
"atom_names": atom_names,
"atom_numbs": atom_numbs,
"atom_types": atom_types,
"coords": np.array([coords]),
"cells": np.array([cells]),
"orig": np.zeros(3),
"nopbc": not pbc,
}
return info_dict
Binary file added tests/pymatgen_data/deepmd/set.000/box.npy
View file
Open in desktop
Binary file not shown.
Binary file added tests/pymatgen_data/deepmd/set.000/coord.npy
View file
Open in desktop
Binary file not shown.
Binary file added tests/pymatgen_data/deepmd/set.000/energy.npy
View file
Open in desktop
Binary file not shown.
Binary file added tests/pymatgen_data/deepmd/set.000/force.npy
View file
Open in desktop
Binary file not shown.
Binary file added tests/pymatgen_data/deepmd/set.000/virial.npy
View file
Open in desktop
Binary file not shown.
98 changes: 98 additions & 0 deletions tests/pymatgen_data/deepmd/type.raw
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
1
1
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
0
3
3
3
3
3
3
3
3
3
3
3
3
3
3
3
3
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
2
4 changes: 4 additions & 0 deletions tests/pymatgen_data/deepmd/type_map.raw
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fe
Li
O
P
16 changes: 15 additions & 1 deletion tests/test_from_pymatgen.py → tests/test_pymatgen_structure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import os
import unittest

from comp_sys import CompSys
from comp_sys import CompSys, IsPBC
from context import dpdata

try:
Expand All @@ -28,5 +28,19 @@ def setUp(self):
self.v_places = 6


@unittest.skipIf(not exist_module, "skip pymatgen")
class TestFormToPytmatgen(unittest.TestCase, CompSys, IsPBC):
def setUp(self):
self.system = dpdata.System("pymatgen_data/deepmd/", fmt="deepmd/npy")
self.system_1 = self.system
self.system_2 = dpdata.System().from_pymatgen_structure(
self.system.to("pymatgen/structure")[0]
)
self.places = 6
self.e_places = 6
self.f_places = 6
self.v_places = 6


if __name__ == "__main__":
unittest.main()
Loading