Skip to content

Commit

Permalink
Modified CifParser.check() as one possible solution for issue materia…
Browse files Browse the repository at this point in the history
…lsproject#3626 (materialsproject#3628)

* Modified check method of CifParser as one possible way to fix issue 3626.

* Reincluded all CifParser check() checks but for relative stoichiometry in case of missing formula keys.

* Added CifParser test for skipping relative stoichiometry check in case of missing formula keys and for appending a warning to self.warnings in this case.

* Refactor CifParser.check

* tighter warning test

---------

Co-authored-by: Janosh Riebesell <[email protected]>
  • Loading branch information
kaueltzen and janosh authored Feb 22, 2024
1 parent 05e8abb commit 57842a7
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 17 deletions.
41 changes: 24 additions & 17 deletions pymatgen/io/cif.py
Original file line number Diff line number Diff line change
Expand Up @@ -1313,13 +1313,6 @@ def has_errors(self):
def check(self, structure: Structure) -> str | None:
"""Check whether a structure constructed from CIF passes sanity checks.
Args:
structure (Structure) : structure created from CIF
Returns:
str | None: If any check fails, on output, returns a human-readable str for the
reason why (e.g., which elements are missing). Returns None if all checks pass.
Checks:
- Composition from CIF is valid
- CIF composition contains only valid elements
Expand All @@ -1329,19 +1322,30 @@ def check(self, structure: Structure) -> str | None:
- CIF and structure have same relative stoichiometry. Thus
if CIF reports stoichiometry LiFeO, and the structure has
composition (LiFeO)4, this check passes.
Args:
structure (Structure) : structure created from CIF
Returns:
str | None: If any check fails, on output, returns a human-readable str for the
reason why (e.g., which elements are missing). Returns None if all checks pass.
"""
failure_reason = None

cif_as_dict = self.as_dict()
head_key = next(iter(cif_as_dict))

cif_formula = None
check_stoichiometry = True
for key in ("_chemical_formula_sum", "_chemical_formula_structural"):
if cif_as_dict[head_key].get(key):
cif_formula = cif_as_dict[head_key][key]
break

# In case of missing CIF formula keys, get non-stoichiometric formula from
# unique sites and skip relative stoichiometry check (added in gh-3628)
if cif_formula is None and cif_as_dict[head_key].get("_atom_site_type_symbol"):
check_stoichiometry = False
cif_formula = " ".join(cif_as_dict[head_key]["_atom_site_type_symbol"])

try:
Expand Down Expand Up @@ -1370,17 +1374,20 @@ def check(self, structure: Structure) -> str | None:
failure_reason = f"Missing elements {missing_str} {addendum}"

elif not all(struct_comp[elt] - orig_comp[elt] == 0 for elt in orig_comp):
# Check that stoichiometry is same, i.e., same relative ratios of elements
ratios = {elt: struct_comp[elt] / orig_comp[elt] for elt in orig_comp_elts}

same_stoich = all(
abs(ratios[elt_a] - ratios[elt_b]) < self.comp_tol
for elt_a in orig_comp_elts
for elt_b in orig_comp_elts
)
if check_stoichiometry:
# Check that CIF/PMG stoichiometry has same relative ratios of elements
ratios = {elt: struct_comp[elt] / orig_comp[elt] for elt in orig_comp_elts}

same_stoich = all(
abs(ratios[elt_a] - ratios[elt_b]) < self.comp_tol
for elt_a in orig_comp_elts
for elt_b in orig_comp_elts
)

if not same_stoich:
failure_reason = f"Incorrect stoichiometry:\n CIF={orig_comp}\n PMG={struct_comp}\n {ratios=}"
if not same_stoich:
failure_reason = f"Incorrect stoichiometry:\n CIF={orig_comp}\n PMG={struct_comp}\n {ratios=}"
else:
self.warnings += ["Skipping relative stoichiometry check because CIF does not contain formula keys."]

return failure_reason

Expand Down
9 changes: 9 additions & 0 deletions tests/io/test_cif.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ def test_cif_parser(self):

with open(f"{TEST_FILES_DIR}/FePO4.cif") as cif_file:
cif_str = cif_file.read()

parser = CifParser.from_str(cif_str)
struct = parser.parse_structures(primitive=False)[0]
assert struct.formula == "Fe4 P4 O16"
Expand Down Expand Up @@ -933,6 +934,14 @@ def test_invalid_cif_composition(self):
failure_reason = cif.check(Structure.from_file(f"{TEST_FILES_DIR}/LiFePO4.cif"))
assert failure_reason == "'X' is not a valid Element"

def test_skipping_relative_stoichiometry_check(self):
cif = CifParser(f"{TEST_FILES_DIR}/Li10GeP2S12.cif")
struct = cif.parse_structures()[0]
failure_reason = cif.check(struct)
assert failure_reason is None
assert len(cif.warnings) == 2
assert cif.warnings[-1] == "Skipping relative stoichiometry check because CIF does not contain formula keys."

def test_cif_writer_site_properties(self):
# check CifWriter(write_site_properties=True) adds Structure site properties to
# CIF with _atom_site_ prefix
Expand Down

0 comments on commit 57842a7

Please sign in to comment.