Clean up (materialsproject#3010)

* simplify: structure.sites -> structure

* ruff unignore SIM115, fix pymatgen/io/qchem/tests/test_inputs.py

* ruff unignore RET501 and fix unnecessary return none

* ruff unignore and fix PLW3301

* ruff unignore PLW0603 and fix use of globals

* improve TestCRESTOutput readability

* ruff unignore PD002 (pandas don't use inplace)

* fix LammpsDataTest.test_from_structure()

>           mol_ids.extend([i + 1] * len(topo))
E           TypeError: object of type 'Topology' has no len()

pymatgen/io/lammps/data.py:786: TypeError

Author: janosh

docs_rst/conf-normal.py

@@ -348,7 +348,7 @@ def find_source():
         filename = f"pymatgen/core/{rel_path}#L{line_start}-L{line_end}"
     except Exception:
         # no need to be relative to core here as module includes full path.
-        filename = info["module"].replace(".", "/") + ".py"
+        filename = f"{info['module'].replace('.', '/')}.py"
 
     tag = "v" + __version__
     return f"https://github.com/materialsproject/pymatgen/blob/{tag}/{filename}"

pymatgen/alchemy/tests/test_materials.py

@@ -63,9 +63,10 @@ def test_final_structure(self):
         assert self.trans.final_structure.composition.reduced_formula == "NaFePO4"
 
     def test_from_dict(self):
-        d = json.load(open(os.path.join(PymatgenTest.TEST_FILES_DIR, "transformations.json")))
-        d["other_parameters"] = {"tags": ["test"]}
-        ts = TransformedStructure.from_dict(d)
+        with open(os.path.join(PymatgenTest.TEST_FILES_DIR, "transformations.json")) as file:
+            dct = json.load(file)
+        dct["other_parameters"] = {"tags": ["test"]}
+        ts = TransformedStructure.from_dict(dct)
         ts.other_parameters["author"] = "Will"
         ts.append_transformation(SubstitutionTransformation({"Fe": "Mn"}))
         assert ts.final_structure.composition.reduced_formula == "MnPO4"

pymatgen/analysis/bond_valence.py

@@ -295,7 +295,7 @@ def _recurse(assigned=None):
                 # recurses to find permutations of valences based on whether a
                 # charge balanced assignment can still be found
                 if self._n > self.max_permutations:
-                    return None
+                    return
                 if assigned is None:
                     assigned = []
 
@@ -312,16 +312,16 @@ def _recurse(assigned=None):
 
                 if highest < 0 or lowest > 0:
                     self._n += 1
-                    return None
+                    return
 
                 if i == len(valences):
                     evaluate_assignment(assigned)
                     self._n += 1
-                    return None
+                    return
                 for v in valences[i]:
                     new_assigned = list(assigned)
                     _recurse([*new_assigned, v])
-                return None
+                return
 
         else:
             n_sites = np.array([len(i) for i in equi_sites])
@@ -373,7 +373,7 @@ def _recurse(assigned=None):
                 # recurses to find permutations of valences based on whether a
                 # charge balanced assignment can still be found
                 if self._n > self.max_permutations:
-                    return None
+                    return
                 if assigned is None:
                     assigned = []
 
@@ -392,18 +392,18 @@ def _recurse(assigned=None):
 
                 if highest < -self.charge_neutrality_tolerance or lowest > self.charge_neutrality_tolerance:
                     self._n += 1
-                    return None
+                    return
 
                 if i == len(new_valences):
                     evaluate_assignment(assigned)
                     self._n += 1
-                    return None
+                    return
 
                 for v in new_valences[i]:
                     new_assigned = list(assigned)
                     _recurse([*new_assigned, v])
 
-                return None
+                return
 
         _recurse()
 

pymatgen/analysis/chemenv/coordination_environments/chemenv_strategies.py

@@ -254,57 +254,52 @@ def equivalent_site_index_and_transform(self, psite):
                         isite = isite2
                         break
         # Get the translation between psite and its corresponding site in the unit cell (Translation I)
-        thissite = self.structure_environments.structure[isite]
-        dthissite = psite.frac_coords - thissite.frac_coords
+        this_site = self.structure_environments.structure[isite]
+        dthis_site = psite.frac_coords - this_site.frac_coords
         # Get the translation between the equivalent site for which the neighbors have been computed and the site in
         # the unit cell that corresponds to psite (Translation II)
-        equivsite = self.structure_environments.structure[self.structure_environments.sites_map[isite]].to_unit_cell()
+        equiv_site = self.structure_environments.structure[self.structure_environments.sites_map[isite]].to_unit_cell()
         # equivsite = self.structure_environments.structure[self.structure_environments.sites_map[isite]]
         dequivsite = (
             self.structure_environments.structure[self.structure_environments.sites_map[isite]].frac_coords
-            - equivsite.frac_coords
+            - equiv_site.frac_coords
         )
         found = False
         # Find the symmetry that applies the site in the unit cell to the equivalent site, as well as the translation
         # that gets back the site to the unit cell (Translation III)
         # TODO: check that these tolerances are needed, now that the structures are refined before analyzing envs
         tolerances = [1e-8, 1e-7, 1e-6, 1e-5, 1e-4]
         for tolerance in tolerances:
-            for symop in self.symops:
-                newsite = PeriodicSite(
-                    equivsite._species,
-                    symop.operate(equivsite.frac_coords),
-                    equivsite._lattice,
+            for sym_op in self.symops:
+                new_site = PeriodicSite(
+                    equiv_site._species,
+                    sym_op.operate(equiv_site.frac_coords),
+                    equiv_site._lattice,
                 )
-                if newsite.is_periodic_image(thissite, tolerance=tolerance):
-                    mysym = symop
-                    dthissite2 = thissite.frac_coords - newsite.frac_coords
+                if new_site.is_periodic_image(this_site, tolerance=tolerance):
+                    my_sym = sym_op
+                    dthissite2 = this_site.frac_coords - new_site.frac_coords
                     found = True
                     break
             if not found:
-                symops = [SymmOp.from_rotation_and_translation()]
-                for symop in symops:
-                    newsite = PeriodicSite(
-                        equivsite._species,
-                        symop.operate(equivsite.frac_coords),
-                        equivsite._lattice,
+                sym_ops = [SymmOp.from_rotation_and_translation()]
+                for sym_op in sym_ops:
+                    new_site = PeriodicSite(
+                        equiv_site._species,
+                        sym_op.operate(equiv_site.frac_coords),
+                        equiv_site._lattice,
                     )
                     # if newsite.is_periodic_image(thissite):
-                    if newsite.is_periodic_image(thissite, tolerance=tolerance):
-                        mysym = symop
-                        dthissite2 = thissite.frac_coords - newsite.frac_coords
+                    if new_site.is_periodic_image(this_site, tolerance=tolerance):
+                        my_sym = sym_op
+                        dthissite2 = this_site.frac_coords - new_site.frac_coords
                         found = True
                         break
             if found:
                 break
         if not found:
             raise EquivalentSiteSearchError(psite)
-        return [
-            self.structure_environments.sites_map[isite],
-            dequivsite,
-            dthissite + dthissite2,
-            mysym,
-        ]
+        return [self.structure_environments.sites_map[isite], dequivsite, dthis_site + dthissite2, my_sym]
 
     @abc.abstractmethod
     def get_site_neighbors(self, site):

pymatgen/analysis/chemenv/coordination_environments/structure_environments.py

@@ -677,12 +677,12 @@ def plot_csm_and_maps(self, isite, max_csm=8.0):
             import matplotlib.pyplot as plt
         except ImportError:
             print('Plotting Chemical Environments requires matplotlib ... exiting "plot" function')
-            return None
+            return
         fig = self.get_csm_and_maps(isite=isite, max_csm=max_csm)
         if fig is None:
-            return None
+            return
         plt.show()
-        return None
+        return
 
     def get_csm_and_maps(self, isite, max_csm=8.0, figsize=None, symmetry_measure_type=None):
         """
@@ -728,7 +728,7 @@ def get_csm_and_maps(self, isite, max_csm=8.0, figsize=None, symmetry_measure_ty
                 if len(min_geoms) == 0:
                     continue
                 wds = nb_set.normalized_distances
-                max_wd = max(max_wd, max(wds))
+                max_wd = max(max_wd, *wds)
                 all_wds.append(wds)
                 all_was.append(nb_set.normalized_angles)
                 for mp_symbol, cg_dict in min_geoms:

pymatgen/analysis/interfaces/substrate_analyzer.py

@@ -52,7 +52,7 @@ def from_zsl(
         if elasticity_tensor is not None:
             energy_density = elasticity_tensor.energy_density(strain)
 
-            elastic_energy = film.volume * energy_density / len(film.sites)
+            elastic_energy = film.volume * energy_density / len(film)
         else:
             elastic_energy = 0
 

pymatgen/analysis/local_env.py

@@ -3310,7 +3310,7 @@ def get_order_parameters(
                                 qsp_theta[i][j][k] = (
                                     qsp_theta[i][j][k] / norms[i][j][k] if norms[i][j][k] > 1.0e-12 else 0.0
                                 )
-                            ops[i] = max(qsp_theta[i][j]) if j == 0 else max(ops[i], max(qsp_theta[i][j]))
+                            ops[i] = max(qsp_theta[i][j]) if j == 0 else max(ops[i], *qsp_theta[i][j])
                 elif t == "bcc":
                     ops[i] = 0.0
                     for j in range(nneigh):
@@ -3327,7 +3327,7 @@ def get_order_parameters(
                             tmp = self._params[i][2] * (d - dmean)
                             acc = acc + exp(-0.5 * tmp * tmp)
                         for j in range(nneigh):
-                            ops[i] = max(qsp_theta[i][j]) if j == 0 else max(ops[i], max(qsp_theta[i][j]))
+                            ops[i] = max(qsp_theta[i][j]) if j == 0 else max(ops[i], *qsp_theta[i][j])
                         ops[i] = acc * ops[i] / float(nneigh)
                         # nneigh * (nneigh - 1))
                     else:

pymatgen/analysis/magnetism/analyzer.py

@@ -609,11 +609,7 @@ def __init__(
         self,
         structure: Structure,
         default_magmoms: dict[str, float] | None = None,
-        strategies: list[str]
-        | tuple[str, ...] = (
-            "ferromagnetic",
-            "antiferromagnetic",
-        ),
+        strategies: list[str] | tuple[str, ...] = ("ferromagnetic", "antiferromagnetic"),
         automatic: bool = True,
         truncate_by_symmetry: bool = True,
         transformation_kwargs: dict | None = None,
@@ -706,7 +702,11 @@ def __init__(
         # we will first create a set of transformations
         # and then apply them to our input structure
         self.transformations = self._generate_transformations(self.sanitized_structure)
-        self._generate_ordered_structures(self.sanitized_structure, self.transformations)
+        ordered_structures, ordered_structures_origins = self._generate_ordered_structures(
+            self.sanitized_structure, self.transformations
+        )
+        self.ordered_structures = ordered_structures
+        self.ordered_structure_origins = ordered_structures_origins
 
     @staticmethod
     def _sanitize_input_structure(input_structure: Structure) -> Structure:
@@ -934,7 +934,7 @@ def _generate_ordered_structures(
         self,
         sanitized_input_structure: Structure,
         transformations: dict[str, MagOrderingTransformation],
-    ):
+    ) -> tuple[list[Structure], list[str]]:
         """Apply our input structure to our list of transformations and output a list
         of ordered structures that have been pruned for duplicates and for those
         with low symmetry (optional).
@@ -1056,9 +1056,7 @@ def _add_structures(ordered_structures, ordered_structures_origins, structures_t
                 self.logger.info(f"Input structure was found in enumerated structures at index {matches.index(True)}")
                 self.input_index = matches.index(True)
                 self.input_origin = ordered_structures_origins[self.input_index]
-
-        self.ordered_structures = ordered_structures
-        self.ordered_structure_origins = ordered_structures_origins
+        return ordered_structures, ordered_structures_origins
 
 
 MagneticDeformation = namedtuple("MagneticDeformation", "type deformation")

pymatgen/analysis/tests/test_adsorption.py

@@ -105,15 +105,15 @@ def test_generate_adsorption_structures(self):
         # Check translation
         sites = self.asf_211.find_adsorption_sites()
         ads_site_coords = sites["all"][0]
-        c_site = structures[0].sites[-2]
+        c_site = structures[0][-2]
         assert str(c_site.specie) == "C"
         self.assertArrayAlmostEqual(c_site.coords, sites["all"][0])
         # Check no translation
         structures = self.asf_111.generate_adsorption_structures(co, translate=False)
         assert co == Molecule("CO", [[1.0, -0.5, 3], [0.8, 0.46, 3.75]])
         sites = self.asf_111.find_adsorption_sites()
         ads_site_coords = sites["all"][0]
-        c_site = structures[0].sites[-2]
+        c_site = structures[0][-2]
         self.assertArrayAlmostEqual(c_site.coords, ads_site_coords + np.array([1.0, -0.5, 3]))
 
     def test_adsorb_both_surfaces(self):

pymatgen/command_line/enumlib_caller.py

@@ -205,7 +205,7 @@ def get_sg_info(ss):
             finder = SpacegroupAnalyzer(Structure.from_sites(ss), self.symm_prec)
             return finder.get_space_group_number()
 
-        target_sgnum = get_sg_info(symmetrized_structure.sites)
+        target_sgnum = get_sg_info(list(symmetrized_structure))
         curr_sites = list(itertools.chain.from_iterable(disordered_sites))
         sgnum = get_sg_info(curr_sites)
         ordered_sites = sorted(ordered_sites, key=lambda sites: len(sites))
@@ -250,12 +250,12 @@ def get_sg_info(ss):
         output.append(str(self.enum_precision_parameter))
         output.append("full")
 
-        ndisordered = sum(len(s) for s in disordered_sites)
+        n_disordered = sum(len(s) for s in disordered_sites)
         base = int(
-            ndisordered
+            n_disordered
             * lcm(
                 *(
-                    f.limit_denominator(ndisordered * self.max_cell_size).denominator
+                    f.limit_denominator(n_disordered * self.max_cell_size).denominator
                     for f in map(fractions.Fraction, index_amounts)
                 )
             )
@@ -268,7 +268,7 @@ def get_sg_info(ss):
         # enumeration. See Cu7Te5.cif test file.
         base *= 10
 
-        # base = ndisordered #10 ** int(math.ceil(math.log10(ndisordered)))
+        # base = ndisordered # 10 ** int(math.ceil(math.log10(ndisordered)))
         # To get a reasonable number of structures, we fix concentrations to the
         # range expected in the original structure.
         total_amounts = sum(index_amounts)

pymatgen/command_line/tests/test_gulp_caller.py

@@ -306,7 +306,7 @@ def test_get_energy_relax_structure_buckingham(self):
         assert isinstance(energy, float)
         assert isinstance(struct, Structure)
         site_len = len(struct)
-        assert site_len == len(self.mgo_uc.sites)
+        assert site_len == len(self.mgo_uc)
 
 
 @unittest.skipIf(not gulp_present, "gulp not present.")

pymatgen/core/structure.py

@@ -436,7 +436,7 @@ def is_valid(self, tol: float = DISTANCE_TOLERANCE) -> bool:
             (bool) True if SiteCollection does not contain atoms that are too
             close together.
         """
-        if len(self.sites) == 1:
+        if len(self) == 1:
             return True
         all_dists = self.distance_matrix[np.triu_indices(len(self), 1)]
         return bool(np.min(all_dists) > tol)
@@ -480,7 +480,7 @@ def add_site_property(self, property_name: str, values: list):
             values (list): A sequence of values. Must be same length as
                 number of sites.
         """
-        if len(values) != len(self.sites):
+        if len(values) != len(self):
             raise ValueError("Values must be same length as sites.")
         for site, val in zip(self.sites, values):
             site.properties[property_name] = val
@@ -551,8 +551,11 @@ def add_oxidation_state_by_site(self, oxidation_states: list[float]):
             oxidation_states (list): List of oxidation states.
                 E.g., [1, 1, 1, 1, 2, 2, 2, 2, 5, 5, 5, 5, -2, -2, -2, -2]
         """
-        if len(oxidation_states) != len(self.sites):
-            raise ValueError("Oxidation states of all sites must be specified.")
+        if len(oxidation_states) != len(self):
+            raise ValueError(
+                f"Oxidation states of all sites must be specified, expected {len(self)} values, "
+                f"got {len(oxidation_states)}"
+            )
         for site, ox in zip(self.sites, oxidation_states):
             new_sp = {}
             for el, occu in site.species.items():
@@ -613,8 +616,8 @@ def add_spin_by_site(self, spins: list[float]):
             spins (list): List of spins
                 E.g., [+5, -5, 0, 0]
         """
-        if len(spins) != len(self.sites):
-            raise ValueError("Spin of all sites must be specified in the dictionary.")
+        if len(spins) != len(self):
+            raise ValueError(f"Spins for all sites must be specified, expected {len(self)} spins, got {len(spins)}")
 
         for site, spin in zip(self.sites, spins):
             new_sp = {}

pymatgen/core/tests/test_structure.py

@@ -998,14 +998,14 @@ def test_rotate_sites(self):
         self.structure.rotate_sites(
             indices=[1],
             theta=2.0 * np.pi / 3.0,
-            anchor=self.structure.sites[0].coords,
+            anchor=self.structure[0].coords,
             to_unit_cell=False,
         )
         self.assertArrayAlmostEqual(self.structure.frac_coords[1], [-1.25, 1.5, 0.75], decimal=6)
         self.structure.rotate_sites(
             indices=[1],
             theta=2.0 * np.pi / 3.0,
-            anchor=self.structure.sites[0].coords,
+            anchor=self.structure[0].coords,
             to_unit_cell=True,
         )
         self.assertArrayAlmostEqual(self.structure.frac_coords[1], [0.75, 0.5, 0.75], decimal=6)