Add CifParser.get_structures(on_error='warn') (#3175)

* simplify key in dict check to dict.get(key)

* better var names

* CifParser.get_structures() add kwarg on_error: Literal["ignore", "warn", "raise"] = "warn"

Author: janosh

pymatgen/core/composition.py

@@ -856,16 +856,16 @@ def add_charges_from_oxi_state_guesses(
 
     def remove_charges(self) -> Composition:
         """
-        Removes the charges from any species in a Composition object.
+        Returns a new Composition with charges from each Species removed.
 
         Returns:
             Composition object without charge decoration, for example
             {"Fe3+": 2.0, "O2-":3.0} becomes {"Fe": 2.0, "O":3.0}
         """
-        d: dict[Element, float] = collections.defaultdict(float)
-        for e, a in self.items():
-            d[Element(e.symbol)] += a
-        return Composition(d)
+        dct: dict[Element, float] = collections.defaultdict(float)
+        for specie, amt in self.items():
+            dct[Element(specie.symbol)] += amt
+        return Composition(dct)
 
     def _get_oxid_state_guesses(self, all_oxi_states, max_sites, oxi_states_override, target_charge):
         """

pymatgen/core/ion.py

@@ -142,7 +142,7 @@ def get_reduced_formula_and_factor(self, iupac_ordering: bool = False, hydrates:
         d = {k: int(round(v)) for k, v in comp.get_el_amt_dict().items()}
         (formula, factor) = reduce_formula(d, iupac_ordering=iupac_ordering)
 
-        if "HO" in formula and self.composition["H"] == self.composition["O"]:
+        if self.composition.get("H") == self.composition.get("O") is not None:
             formula = formula.replace("HO", "OH")
 
         if nH2O > 0:

pymatgen/electronic_structure/bandstructure.py

@@ -681,7 +681,7 @@ def from_old_dict(cls, dct):
         labels_dict = {k.strip(): v for k, v in dct["labels_dict"].items()}
         projections = {}
         structure = None
-        if "projections" in dct and len(dct["projections"]) != 0:
+        if dct.get("projections"):
             structure = Structure.from_dict(dct["structure"])
             projections = {}
             for spin in dct["projections"]:

pymatgen/ext/optimade.py

@@ -320,15 +320,13 @@ def get_snls_with_filter(
                 pbar = tqdm(total=json["meta"].get("data_returned", 0), desc=identifier, initial=len(structures))
 
                 # TODO: check spec for `more_data_available` boolean, may simplify this conditional
-                if ("links" in json) and ("next" in json["links"]) and (json["links"]["next"]):
-                    while "next" in json["links"] and json["links"]["next"]:
-                        next_link = json["links"]["next"]
-                        if isinstance(next_link, dict) and "href" in next_link:
-                            next_link = next_link["href"]
-                        json = self._get_json(next_link)
-                        additional_structures = self._get_snls_from_resource(json, url, identifier)
-                        structures.update(additional_structures)
-                        pbar.update(len(additional_structures))
+                while next_link := json.get("links", {}).get("next"):
+                    if isinstance(next_link, dict) and "href" in next_link:
+                        next_link = next_link["href"]
+                    json = self._get_json(next_link)
+                    additional_structures = self._get_snls_from_resource(json, url, identifier)
+                    structures.update(additional_structures)
+                    pbar.update(len(additional_structures))
 
                 if structures:
                     all_snls[identifier] = structures

pymatgen/io/abinit/pseudos.py

@@ -267,7 +267,7 @@ def from_dict(cls, dct):
         new = cls.from_file(dct["filepath"])
 
         # Consistency test based on md5
-        if "md5" in dct and dct["md5"] != new.md5:
+        if dct.get("md5") != new.md5:
             raise ValueError(
                 f"The md5 found in file does not agree with the one in dict\nReceived {dct['md5']}\nComputed {new.md5}"
             )

pymatgen/io/cif.py

@@ -13,6 +13,7 @@
 from io import StringIO
 from itertools import groupby
 from pathlib import Path
+from typing import Literal
 
 import numpy as np
 from monty.io import zopen
@@ -885,8 +886,8 @@ def _parse_symbol(self, sym):
 
         return parsed_sym
 
-    def _get_structure(self, data, primitive, symmetrized):
-        """Generate structure from part of the cif."""
+    def _get_structure(self, data, primitive, symmetrized) -> Structure | None:
+        """Generate structure from part of the CIF."""
 
         def get_num_implicit_hydrogens(sym):
             num_h = {"Wat": 2, "wat": 2, "O-H": 1}
@@ -907,26 +908,23 @@ def get_num_implicit_hydrogens(sym):
 
         oxi_states = self.parse_oxi_states(data)
 
-        coord_to_species = {}
+        coord_to_species = {}  # type: ignore
         coord_to_magmoms = {}
         labels = {}
 
         def get_matching_coord(coord):
             keys = list(coord_to_species)
             coords = np.array(keys)
             for op in self.symmetry_operations:
-                c = op.operate(coord)
-                inds = find_in_coord_list_pbc(coords, c, atol=self._site_tolerance)
-                # can't use if inds, because python is dumb and np.array([0]) evaluates
-                # to False
-                if len(inds) > 0:
-                    return keys[inds[0]]
+                frac_coord = op.operate(coord)
+                indices = find_in_coord_list_pbc(coords, frac_coord, atol=self._site_tolerance)
+                if len(indices) > 0:
+                    return keys[indices[0]]
             return False
 
         for idx, label in enumerate(data["_atom_site_label"]):
             try:
-                # If site type symbol exists, use it. Otherwise, we use the
-                # label.
+                # If site type symbol exists, use it. Otherwise, we use the label.
                 symbol = self._parse_symbol(data["_atom_site_type_symbol"][idx])
                 num_h = get_num_implicit_hydrogens(data["_atom_site_type_symbol"][idx])
             except KeyError:
@@ -946,7 +944,7 @@ def get_matching_coord(coord):
                 except Exception:
                     el = DummySpecies(symbol, o_s)
             else:
-                el = get_el_sp(symbol)
+                el = get_el_sp(symbol)  # type: ignore
 
             x = str2float(data["_atom_site_fract_x"][idx])
             y = str2float(data["_atom_site_fract_y"][idx])
@@ -963,7 +961,7 @@ def get_matching_coord(coord):
                 match = get_matching_coord(coord)
                 comp_d = {el: occu}
                 if num_h > 0:
-                    comp_d["H"] = num_h
+                    comp_d["H"] = num_h  # type: ignore
                     self.warnings.append(
                         "Structure has implicit hydrogens defined, "
                         "parsed structure unlikely to be suitable for use "
@@ -1067,12 +1065,12 @@ def get_matching_coord(coord):
                 site_properties["magmom"] = all_magmoms
 
             if len(site_properties) == 0:
-                site_properties = None
+                site_properties = None  # type: ignore
 
             if any(all_labels):
                 assert len(all_labels) == len(all_species)
             else:
-                all_labels = None
+                all_labels = None  # type: ignore
 
             struct = Structure(lattice, all_species, all_coords, site_properties=site_properties, labels=all_labels)
 
@@ -1099,10 +1097,10 @@ def get_matching_coord(coord):
             return struct
         return None
 
-    def get_structures(self, primitive=True, symmetrized=False):
-        """
-        Return list of structures in CIF file. primitive boolean sets whether a
-        conventional cell structure or primitive cell structure is returned.
+    def get_structures(
+        self, primitive: bool = True, symmetrized: bool = False, on_error: Literal["ignore", "warn", "raise"] = "warn"
+    ) -> list[Structure]:
+        """Return list of structures in CIF file.
 
         Args:
             primitive (bool): Set to False to return conventional unit cells.
@@ -1116,9 +1114,11 @@ def get_structures(self, primitive=True, symmetrized=False):
                 currently Wyckoff labels and space group labels or numbers are
                 not included in the generated SymmetrizedStructure, these will be
                 notated as "Not Parsed" or -1 respectively.
+            on_error ('ignore' | 'warn' | 'raise'): What to do in case of KeyError or ValueError
+                while parsing CIF file. Defaults to 'warn'.
 
         Returns:
-            List of Structures.
+            list[Structure]: All structures in CIF file.
         """
         if primitive and symmetrized:
             raise ValueError(
@@ -1127,24 +1127,28 @@ def get_structures(self, primitive=True, symmetrized=False):
             )
 
         structures = []
-        for i, d in enumerate(self._cif.data.values()):
+        for idx, dct in enumerate(self._cif.data.values()):
             try:
-                struct = self._get_structure(d, primitive, symmetrized)
+                struct = self._get_structure(dct, primitive, symmetrized)
                 if struct:
                     structures.append(struct)
             except (KeyError, ValueError) as exc:
-                # Warn the user (Errors should never pass silently)
                 # A user reported a problem with cif files produced by Avogadro
                 # in which the atomic coordinates are in Cartesian coords.
-                self.warnings.append(str(exc))
-                warnings.warn(f"No structure parsed for {i + 1} structure in CIF. Section of CIF file below.")
-                warnings.warn(str(d))
-                warnings.warn(f"Error is {exc}.")
+                msg = f"No structure parsed for section {idx + 1} in CIF.\n{exc}"
+                if on_error == "raise":
+                    raise ValueError(msg) from exc
+                if on_error == "warn":
+                    warnings.warn(msg)
+                self.warnings.append(msg)
+                # continue silently if on_error == "ignore"
 
-        if self.warnings:
+        # if on_error == "raise" we don't get to here so no need to check
+        if self.warnings and on_error == "warn":
             warnings.warn("Issues encountered while parsing CIF: " + "\n".join(self.warnings))
+
         if len(structures) == 0:
-            raise ValueError("Invalid cif file with no structures!")
+            raise ValueError("Invalid CIF file with no structures!")
         return structures
 
     def get_bibtex_string(self):

pymatgen/io/lmto.py

@@ -289,13 +289,7 @@ def from_dict(cls, dct):
                     dct["SPCGRP"], plat, species, positions, coords_are_cartesian=True
                 )
             except ValueError:
-                structure = Structure(
-                    plat,
-                    species,
-                    positions,
-                    coords_are_cartesian=True,
-                    to_unit_cell=True,
-                )
+                structure = Structure(plat, species, positions, coords_are_cartesian=True, to_unit_cell=True)
         else:
             structure = Structure(plat, species, positions, coords_are_cartesian=True, to_unit_cell=True)
 

pymatgen/io/tests/test_cif.py

@@ -26,10 +26,10 @@
 
 class CifBlockTest(PymatgenTest):
     def test_to_string(self):
-        with open(f"{self.TEST_FILES_DIR}/Graphite.cif") as f:
-            s = f.read()
-        c = CifBlock.from_str(s)
-        cif_str_2 = str(CifBlock.from_str(str(c)))
+        with open(f"{self.TEST_FILES_DIR}/Graphite.cif") as file:
+            cif_str = file.read()
+        cif_block = CifBlock.from_str(cif_str)
+        cif_str_2 = str(CifBlock.from_str(str(cif_block)))
         cif_str = """data_53781-ICSD
 _database_code_ICSD   53781
 _audit_creation_date   2003-04-01
@@ -108,7 +108,7 @@ def test_to_string(self):
  _atom_site_attached_hydrogens
   C1  C0+  2  b  0  0  0.25  .  1.  0
   C2  C0+  2  c  0.3333  0.6667  0.25  .  1.  0"""
-        for l1, l2, l3 in zip(str(c).split("\n"), cif_str.split("\n"), cif_str_2.split("\n")):
+        for l1, l2, l3 in zip(str(cif_block).split("\n"), cif_str.split("\n"), cif_str_2.split("\n")):
             assert l1.strip() == l2.strip()
             assert l2.strip() == l3.strip()
 
@@ -717,7 +717,7 @@ def test_no_coords_or_species(self):
       ? ? ? ? ? ? ?
     """
         parser = CifParser.from_str(string)
-        with pytest.raises(ValueError, match="Invalid cif file with no structures"):
+        with pytest.raises(ValueError, match="Invalid CIF file with no structures"):
             parser.get_structures()
 
     def test_get_lattice_from_lattice_type(self):
@@ -792,11 +792,13 @@ def test_empty(self):
         assert cb == cb2
 
     def test_bad_cif(self):
-        f = f"{self.TEST_FILES_DIR}/bad_occu.cif"
-        parser = CifParser(f)
-        with pytest.raises(ValueError, match="Invalid cif file with no structures"):
-            parser.get_structures()
-        parser = CifParser(f, occupancy_tolerance=2)
+        filepath = f"{self.TEST_FILES_DIR}/bad_occu.cif"
+        parser = CifParser(filepath)
+        with pytest.raises(
+            ValueError, match="No structure parsed for section 1 in CIF.\nSpecies occupancies sum to more than 1!"
+        ):
+            parser.get_structures(on_error="raise")
+        parser = CifParser(filepath, occupancy_tolerance=2)
         struct = parser.get_structures()[0]
         assert struct[0].species["Al3+"] == approx(0.5)
 
@@ -829,7 +831,7 @@ def test_replacing_finite_precision_frac_coords(self):
         )
 
     def test_empty_deque(self):
-        s = """data_1526655
+        cif_str = """data_1526655
 _journal_name_full
 _space_group_IT_number           227
 _symmetry_space_group_name_Hall  'F 4d 2 3 -1d'
@@ -862,7 +864,7 @@ def test_empty_deque(self):
   3  -x,-y,-z
   4  x-1/2,-y-1/2,z-1/2
 ;"""
-        parser = CifParser.from_str(s)
+        parser = CifParser.from_str(cif_str)
         assert parser.get_structures()[0].formula == "Si1"
         cif = """
 data_1526655
@@ -894,7 +896,7 @@ def test_empty_deque(self):
 Si1 Si 0 0 0 1 0.0
 """
         parser = CifParser.from_str(cif)
-        with pytest.raises(ValueError, match="Invalid cif file with no structures"):
+        with pytest.raises(ValueError, match="Invalid CIF file with no structures"):
             parser.get_structures()