Skip to content

Commit

Permalink
Add min "thickness" check in CifParser to filter invalid structure …
Browse files Browse the repository at this point in the history
…which leads to infinite loop (#4133)

* add min_vol check

* increase min_vol to 1

* round volume to 4 digits

* add test

* use min thickness for robustness

* compress test file

* lower the threshold even further

* update comment
  • Loading branch information
DanielYang59 authored Nov 13, 2024
1 parent 40100e9 commit 65f52ea
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 1 deletion.
22 changes: 21 additions & 1 deletion src/pymatgen/io/cif.py
Original file line number Diff line number Diff line change
Expand Up @@ -969,8 +969,22 @@ def _get_structure(
primitive: bool,
symmetrized: bool,
check_occu: bool = False,
min_thickness: float = 0.01,
) -> Structure | None:
"""Generate structure from part of the CIF."""
"""Generate structure from part of the CIF.
Args:
data (CifBlock): The data block to parse.
primitive (bool): Whether to return primitive unit cells.
symmetrized (bool): Whether to return SymmetrizedStructure.
check_occu (bool): Whether to check site for unphysical occupancy > 1.
min_thickness (float): Minimum thickness in Angstrom to consider structure as valid.
This is added to guard against unphysical small/thin structure,
which could result in infinite loop for searching near neighbours.
Returns:
Structure or None if not found.
"""

def get_num_implicit_hydrogens(symbol: str) -> int:
"""Get number of implicit hydrogens."""
Expand All @@ -992,6 +1006,12 @@ def get_matching_coord(

lattice = self.get_lattice(data)

# Check minimal lattice thickness
if lattice is not None:
thickness = [lattice.d_hkl((1, 0, 0)), lattice.d_hkl((0, 1, 0)), lattice.d_hkl((0, 0, 1))]
if any(t < min_thickness for t in thickness):
raise ValueError(f"{thickness=} Å below threshold, double check your structure.")

# If magCIF, get magnetic symmetry moments and magmoms
# else standard CIF, and use empty magmom dict
if self.feature_flags["magcif_incommensurate"]:
Expand Down
Binary file added tests/files/cif/bad_superflat_inf_loop.cif.gz
Binary file not shown.
12 changes: 12 additions & 0 deletions tests/io/test_cif.py
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,18 @@ def test_cif_parser(self):
assert len(parser.parse_structures(primitive=False)[0]) == 2
assert not parser.has_errors

def test_parse_bad_superflat(self):
"""
Test unphysically "flat" structure with volume near zero,
which would originally lead to infinite loop (PR4133).
"""
parser = CifParser(f"{TEST_FILES_DIR}/cif/bad_superflat_inf_loop.cif.gz")
with (
pytest.raises(ValueError, match="Invalid CIF file with no structures"),
pytest.warns(UserWarning, match="Å below threshold, double check your structure."),
):
parser.parse_structures()

def test_get_symmetrized_structure(self):
parser = CifParser(f"{TEST_FILES_DIR}/cif/Li2O.cif")
sym_structure = parser.parse_structures(primitive=False, symmetrized=True)[0]
Expand Down

0 comments on commit 65f52ea

Please sign in to comment.