From c8d41e647c28569365aafe19de9841c3cc7e2e42 Mon Sep 17 00:00:00 2001 From: Janosh Riebesell Date: Sat, 26 Aug 2023 11:02:50 -0700 Subject: [PATCH] `__str__` to `__repr__` (#3274) * use self-documenting f-string in reprs * TestLibxcFunc add test_repr * use python behavior that __str__ falls back to __repr__ if not defined but not vice versa * improve ConversionElectrode.__repr__ and add test * fix test_energy_adjustment_repr --- pymatgen/apps/battery/conversion_battery.py | 13 ++-- pymatgen/core/libxcfunc.py | 9 +-- pymatgen/core/periodic_table.py | 6 +- pymatgen/electronic_structure/cohp.py | 3 - pymatgen/entries/computed_entries.py | 7 +- pymatgen/io/abinit/abitimer.py | 8 +-- pymatgen/io/vasp/inputs.py | 3 - pymatgen/io/wannier90.py | 6 +- pymatgen/symmetry/analyzer.py | 8 +-- .../advanced_transformations.py | 30 ++------- .../transformations/site_transformations.py | 25 ++----- .../standard_transformations.py | 65 ++++--------------- tests/apps/battery/test_conversion_battery.py | 19 ++++-- tests/core/test_libxcfunc.py | 5 ++ tests/entries/test_computed_entries.py | 2 +- tests/io/vasp/test_sets.py | 2 +- 16 files changed, 72 insertions(+), 139 deletions(-) diff --git a/pymatgen/apps/battery/conversion_battery.py b/pymatgen/apps/battery/conversion_battery.py index b2eefc58de6..a1955691619 100644 --- a/pymatgen/apps/battery/conversion_battery.py +++ b/pymatgen/apps/battery/conversion_battery.py @@ -205,12 +205,15 @@ def __hash__(self) -> int: return 7 def __repr__(self): + cls_name, formula, n_steps = type(self).__name__, self.initial_comp.reduced_formula, self.num_steps + avg_voltage, min_voltage, max_voltage = self.get_average_voltage(), self.min_voltage, self.max_voltage output = [ - f"Conversion electrode with formula {self.initial_comp.reduced_formula} and nsteps {self.num_steps}", - f"Avg voltage {self.get_average_voltage()} V, min voltage {self.min_voltage} V, " - f"max voltage {self.max_voltage} V", - f"Capacity (grav.) {self.get_capacity_grav()} mAh/g, capacity (vol.) {self.get_capacity_vol()} Ah/l", - f"Specific energy {self.get_specific_energy()} Wh/kg, energy density {self.get_energy_density()} Wh/l", + f"{cls_name} with {formula=} and {n_steps=}, {avg_voltage=:.3f} V, " + f"{min_voltage=:.3f} V, {max_voltage=:.3f} V", + f"Capacity (grav.) {self.get_capacity_grav():.3f} mAh/g, capacity (vol.) " + f"{self.get_capacity_vol():.3f} Ah/l", + f"Specific energy {self.get_specific_energy():.3f} Wh/kg, energy density " + f"{self.get_energy_density():.3f} Wh/l", ] return "\n".join(output) diff --git a/pymatgen/core/libxcfunc.py b/pymatgen/core/libxcfunc.py index 686f851117e..753a539b18b 100644 --- a/pymatgen/core/libxcfunc.py +++ b/pymatgen/core/libxcfunc.py @@ -409,11 +409,12 @@ def __init__(self, _num): num: Number for the xc. """ info = _all_xcfuncs[self.value] - self.kind = info["Kind"] - self.family = info["Family"] + self.kind = info["Kind"] # type: ignore + self.family = info["Family"] # type: ignore - def __str__(self): - return f"name={self.name}, kind={self.kind}, family={self.family}" + def __repr__(self): + name, kind, family = self.name, self.kind, self.family + return f"{type(self).__name__}({name=}, {kind=}, {family=})" @staticmethod def all_families(): diff --git a/pymatgen/core/periodic_table.py b/pymatgen/core/periodic_table.py index 65d6ca7b076..41bc580040e 100644 --- a/pymatgen/core/periodic_table.py +++ b/pymatgen/core/periodic_table.py @@ -1161,7 +1161,8 @@ def __str__(self): if self.oxi_state is not None: output += f"{formula_double_format(abs(self.oxi_state))}{'+' if self.oxi_state >= 0 else '-'}" if self._spin is not None: - output += f",spin={self._spin}" + spin = self._spin + output += f",{spin=}" return output def to_pretty_string(self) -> str: @@ -1453,7 +1454,8 @@ def __str__(self): if self.oxi_state is not None: output += f"{formula_double_format(abs(self.oxi_state))}{'+' if self.oxi_state >= 0 else '-'}" if self._spin is not None: - output += f",spin={self._spin}" + spin = self._spin + output += f",{spin=}" return output diff --git a/pymatgen/electronic_structure/cohp.py b/pymatgen/electronic_structure/cohp.py index 3bb6be79873..7e16cfaab17 100644 --- a/pymatgen/electronic_structure/cohp.py +++ b/pymatgen/electronic_structure/cohp.py @@ -61,9 +61,6 @@ def __init__(self, efermi, energies, cohp, are_coops=False, are_cobis=False, ico self.icohp = icohp def __repr__(self): - return str(self) - - def __str__(self): """Returns a string that can be easily plotted (e.g. using gnuplot).""" if self.are_coops: cohpstring = "COOP" diff --git a/pymatgen/entries/computed_entries.py b/pymatgen/entries/computed_entries.py index 846146a4f0c..ede9ba26d85 100644 --- a/pymatgen/entries/computed_entries.py +++ b/pymatgen/entries/computed_entries.py @@ -90,15 +90,12 @@ def explain(self): """Return an explanation of how the energy adjustment is calculated.""" def __repr__(self): - name, value, uncertainty = self.name, float(self.value), self.uncertainty + name, value, uncertainty, description = self.name, float(self.value), self.uncertainty, self.description # self.cls might not be a dict if monty decoding is enabled in the new MPRester # which hydrates all dicts with @class and @module keys into classes in which case # we expect a Compatibility subclass generated_by = self.cls.get("@class", "unknown") if isinstance(self.cls, dict) else type(self.cls).__name__ - return ( - f"{type(self).__name__}({name=}, {value=:.3}, {uncertainty=:.3}, " - f"description={self.description}, {generated_by=})" - ) + return f"{type(self).__name__}({name=}, {value=:.3}, {uncertainty=:.3}, {description=}, {generated_by=})" class ConstantEnergyAdjustment(EnergyAdjustment): diff --git a/pymatgen/io/abinit/abitimer.py b/pymatgen/io/abinit/abitimer.py index a55e2912e17..7c0d0ee211f 100644 --- a/pymatgen/io/abinit/abitimer.py +++ b/pymatgen/io/abinit/abitimer.py @@ -650,11 +650,9 @@ def __init__(self, sections, info, cpu_time, wall_time): self.mpi_rank = info["mpi_rank"].strip() self.fname = info["fname"].strip() - def __str__(self): - return ( - f"file={self.fname}, wall_time={self.wall_time:.1f}, " - f"mpi_nprocs={self.mpi_nprocs}, omp_nthreads={self.omp_nthreads}" - ) + def __repr__(self): + file, wall_time, mpi_nprocs, omp_nthreads = self.fname, self.wall_time, self.mpi_nprocs, self.omp_nthreads + return f"{type(self).__name__}({file=}, {wall_time=:.3}, {mpi_nprocs=}, {omp_nthreads=})" @property def ncpus(self): diff --git a/pymatgen/io/vasp/inputs.py b/pymatgen/io/vasp/inputs.py index 2fb744edb9f..bea1ebc4be0 100644 --- a/pymatgen/io/vasp/inputs.py +++ b/pymatgen/io/vasp/inputs.py @@ -1493,9 +1493,6 @@ def write_file(self, filename): f.write(str(self)) def __repr__(self): - return str(self) - - def __str__(self): lines = [self.comment, str(self.num_kpts), self.style.name] style = self.style.name.lower()[0] if style == "l": diff --git a/pymatgen/io/wannier90.py b/pymatgen/io/wannier90.py index 5d8e1fda57e..dfb910ec002 100644 --- a/pymatgen/io/wannier90.py +++ b/pymatgen/io/wannier90.py @@ -155,10 +155,8 @@ def write_file(self, filename: str) -> None: f.write_record(self.data[ib].flatten("F")) def __repr__(self) -> str: - return ( - f"" - ) + ik, nbnd, ncl, ngx, ngy, ngz = self.ik, self.nbnd, self.is_noncollinear, *self.ng + return f"{(type(self).__name__)}({ik=}, {nbnd=}, {ncl=}, {ngx=}, {ngy=}, {ngz=})" def __eq__(self, other: object) -> bool: if not isinstance(other, Unk): diff --git a/pymatgen/symmetry/analyzer.py b/pymatgen/symmetry/analyzer.py index fc98f6b6e00..9d025a162d2 100644 --- a/pymatgen/symmetry/analyzer.py +++ b/pymatgen/symmetry/analyzer.py @@ -247,9 +247,10 @@ def _get_symmetry(self): """ d = spglib.get_symmetry(self._cell, symprec=self._symprec, angle_tolerance=self._angle_tol) if d is None: + symprec = self._symprec raise ValueError( f"Symmetry detection failed for structure with formula {self._structure.formula}. " - f"Try setting symprec={self._symprec} to a different value." + f"Try setting {symprec=} to a different value." ) # Sometimes spglib returns small translation vectors, e.g. # [1e-4, 2e-4, 1e-4] @@ -1686,8 +1687,5 @@ def __init__(self, sch_symbol, operations, tol: float = 0.1): self.sch_symbol = sch_symbol super().__init__(generate_full_symmops(operations, tol)) - def __str__(self): - return self.sch_symbol - def __repr__(self): - return str(self) + return self.sch_symbol diff --git a/pymatgen/transformations/advanced_transformations.py b/pymatgen/transformations/advanced_transformations.py index 368fe4abbd5..46da1bd8545 100644 --- a/pymatgen/transformations/advanced_transformations.py +++ b/pymatgen/transformations/advanced_transformations.py @@ -81,11 +81,8 @@ def apply_transformation(self, structure: Structure): trans = SubstitutionTransformation({self.charge_balance_sp: {self.charge_balance_sp: 1 - removal_fraction}}) return trans.apply_transformation(structure) - def __str__(self): - return f"Charge Balance Transformation : Species to remove = {self.charge_balance_sp}" - def __repr__(self): - return str(self) + return f"Charge Balance Transformation : Species to remove = {self.charge_balance_sp}" @property def inverse(self): @@ -142,11 +139,8 @@ def apply_transformation(self, structure: Structure, return_ranked_list: bool | structures.append({"transformation": t, "structure": t.apply_transformation(structure)}) return structures - def __str__(self): - return f"Super Transformation : Transformations = {' '.join(map(str, self._transformations))}" - def __repr__(self): - return str(self) + return f"Super Transformation : Transformations = {' '.join(map(str, self._transformations))}" @property def inverse(self): @@ -246,11 +240,8 @@ def apply_transformation(self, structure: Structure, return_ranked_list: bool | outputs.append({"structure": new_structure}) return outputs - def __str__(self): - return f"Multiple Substitution Transformation : Substitution on {self.sp_to_replace}" - def __repr__(self): - return str(self) + return f"Multiple Substitution Transformation : Substitution on {self.sp_to_replace}" @property def inverse(self): @@ -489,11 +480,8 @@ def sort_func(s): return self._all_structures[0:num_to_return] return self._all_structures[0]["structure"] - def __str__(self): - return "EnumerateStructureTransformation" - def __repr__(self): - return str(self) + return "EnumerateStructureTransformation" @property def inverse(self): @@ -557,11 +545,8 @@ def apply_transformation(self, structure: Structure, return_ranked_list: bool | outputs.append(output) return outputs - def __str__(self): - return "SubstitutionPredictorTransformation" - def __repr__(self): - return str(self) + return "SubstitutionPredictorTransformation" @property def inverse(self): @@ -2199,11 +2184,8 @@ def apply_transformation(self, structure: Structure) -> Structure: coords_are_cartesian=True, ) - def __str__(self): - return f"{__name__} : rattle_std = {self.rattle_std}" - def __repr__(self): - return str(self) + return f"{__name__} : rattle_std = {self.rattle_std}" @property def inverse(self): diff --git a/pymatgen/transformations/site_transformations.py b/pymatgen/transformations/site_transformations.py index c01429ee5be..92a7cd90f29 100644 --- a/pymatgen/transformations/site_transformations.py +++ b/pymatgen/transformations/site_transformations.py @@ -67,11 +67,8 @@ def apply_transformation(self, structure: Structure): ) return struct.get_sorted_structure() - def __str__(self): - return f"InsertSiteTransformation : species {self.species}, coords {self.coords}" - def __repr__(self): - return str(self) + return f"InsertSiteTransformation : species {self.species}, coords {self.coords}" @property def inverse(self): @@ -114,14 +111,11 @@ def apply_transformation(self, structure: Structure): struct[int(i)] = sp return struct - def __str__(self): + def __repr__(self): return "ReplaceSiteSpeciesTransformation :" + ", ".join( [f"{k}->{v}" + v for k, v in self.indices_species_map.items()] ) - def __repr__(self): - return str(self) - @property def inverse(self): """Return: None.""" @@ -157,11 +151,8 @@ def apply_transformation(self, structure: Structure): struct.remove_sites(self.indices_to_remove) return struct - def __str__(self): - return "RemoveSitesTransformation :" + ", ".join(map(str, self.indices_to_remove)) - def __repr__(self): - return str(self) + return "RemoveSitesTransformation :" + ", ".join(map(str, self.indices_to_remove)) @property def inverse(self): @@ -211,16 +202,13 @@ def apply_transformation(self, structure: Structure): struct.translate_sites(self.indices_to_move, self.translation_vector, self.vector_in_frac_coords) return struct - def __str__(self): + def __repr__(self): return ( f"TranslateSitesTransformation for indices {self.indices_to_move}, " f"vect {self.translation_vector} and " f"vect_in_frac_coords = {self.vector_in_frac_coords}" ) - def __repr__(self): - return str(self) - @property def inverse(self): """ @@ -502,11 +490,8 @@ def apply_transformation(self, structure: Structure, return_ranked_list: bool | opt_s = all_structures[0]["structure"] return opt_s if not return_ranked_list else all_structures[0:num_to_return] - def __str__(self): - return f"PartialRemoveSitesTransformation : Indices and fraction to remove = {self.indices}, ALGO = {self.algo}" - def __repr__(self): - return str(self) + return f"PartialRemoveSitesTransformation : Indices and fraction to remove = {self.indices}, ALGO = {self.algo}" @property def inverse(self): diff --git a/pymatgen/transformations/standard_transformations.py b/pymatgen/transformations/standard_transformations.py index 0d9322c9c71..687fbf1e44c 100644 --- a/pymatgen/transformations/standard_transformations.py +++ b/pymatgen/transformations/standard_transformations.py @@ -61,15 +61,12 @@ def apply_transformation(self, structure): struct.apply_operation(self._symmop) return struct - def __str__(self): + def __repr__(self): return ( f"Rotation Transformation about axis {self.axis} with angle = " f"{self.angle:.4f} {'radians' if self.angle_in_radians else 'degrees'}" ) - def __repr__(self): - return str(self) - @property def inverse(self): """Returns inverse Transformation.""" @@ -291,11 +288,8 @@ def apply_transformation(self, structure): """ return structure * self.scaling_matrix - def __str__(self): - return f"Supercell Transformation with scaling matrix {self.scaling_matrix}" - def __repr__(self): - return str(self) + return f"Supercell Transformation with scaling matrix {self.scaling_matrix}" @property def inverse(self): @@ -347,11 +341,8 @@ def apply_transformation(self, structure: Structure) -> Structure: struct.replace_species(species_map) # type: ignore[arg-type] return struct - def __str__(self): - return "Substitution Transformation :" + ", ".join([f"{k}->{v}" for k, v in self._species_map.items()]) - def __repr__(self): - return str(self) + return "Substitution Transformation :" + ", ".join([f"{k}->{v}" for k, v in self._species_map.items()]) @property def inverse(self): @@ -389,11 +380,8 @@ def apply_transformation(self, structure): struct.remove_species([get_el_sp(sp)]) return struct - def __str__(self): - return "Remove Species Transformation :" + ", ".join(self.species_to_remove) - def __repr__(self): - return str(self) + return "Remove Species Transformation :" + ", ".join(self.species_to_remove) @property def inverse(self): @@ -467,15 +455,12 @@ def is_one_to_many(self) -> bool: """Returns: True.""" return True - def __str__(self): + def __repr__(self): species = self.specie_to_remove fraction_to_remove = self.fraction_to_remove algo = self.algo return f"PartialRemoveSpecieTransformation({species=}, {fraction_to_remove=}, {algo=})" - def __repr__(self): - return str(self) - @property def inverse(self): """Returns: None.""" @@ -653,11 +638,8 @@ def apply_transformation(self, structure: Structure, return_ranked_list: bool | return self._all_structures[:num_to_return] return self._all_structures[0]["structure"] - def __str__(self): - return "Order disordered structure transformation" - def __repr__(self): - return str(self) + return "Order disordered structure transformation" @property def inverse(self): @@ -703,11 +685,8 @@ def apply_transformation(self, structure): """ return structure.get_primitive_structure(tolerance=self.tolerance) - def __str__(self): - return "Primitive cell transformation" - def __repr__(self): - return str(self) + return "Primitive cell transformation" @property def inverse(self): @@ -747,11 +726,8 @@ def apply_transformation(self, structure): sga = SpacegroupAnalyzer(structure, symprec=self.symprec, angle_tolerance=self.angle_tolerance) return sga.get_conventional_standard_structure(international_monoclinic=self.international_monoclinic) - def __str__(self): - return "Conventional cell transformation" - def __repr__(self): - return str(self) + return "Conventional cell transformation" @property def inverse(self): @@ -799,11 +775,8 @@ def apply_transformation(self, structure: Structure) -> Structure: struct.perturb(self.distance, min_distance=self.min_distance) return struct - def __str__(self): - return f"PerturbStructureTransformation : Min_distance = {self.min_distance}" - def __repr__(self): - return str(self) + return f"PerturbStructureTransformation : Min_distance = {self.min_distance}" @property def inverse(self): @@ -838,11 +811,8 @@ def apply_transformation(self, structure): """ return self._deform.apply_to_structure(structure) - def __str__(self): - return f"DeformStructureTransformation : Deformation = {self.deformation}" - def __repr__(self): - return str(self) + return f"DeformStructureTransformation : Deformation = {self.deformation}" @property def inverse(self): @@ -907,11 +877,8 @@ def apply_transformation(self, structure): return Structure(structure.lattice, species, structure.frac_coords) - def __str__(self): - return "DiscretizeOccupanciesTransformation" - def __repr__(self): - return str(self) + return "DiscretizeOccupanciesTransformation" @property def inverse(self): @@ -950,11 +917,8 @@ def apply_transformation(self, structure): struct.set_charge(self.charge) return struct - def __str__(self): - return f"Structure with charge {self.charge}" - def __repr__(self): - return str(self) + return f"Structure with charge {self.charge}" @property def inverse(self): @@ -1026,11 +990,8 @@ def apply_transformation(self, structure): return Structure(new_lattice, species, frac_coords) - def __str__(self): - return "ScaleToRelaxedTransformation" - def __repr__(self): - return str(self) + return "ScaleToRelaxedTransformation" @property def inverse(self): diff --git a/tests/apps/battery/test_conversion_battery.py b/tests/apps/battery/test_conversion_battery.py index 6f5e5ed4940..99a6117b861 100644 --- a/tests/apps/battery/test_conversion_battery.py +++ b/tests/apps/battery/test_conversion_battery.py @@ -15,7 +15,7 @@ class TestConversionElectrode(unittest.TestCase): def setUp(self): self.formulas = ["LiCoO2", "FeF3", "MnO2"] - self.conversion_eletrodes = {} + self.conversion_electrodes = {} for f in self.formulas: with open(os.path.join(TEST_FILES_DIR, f + "_batt.json")) as fid: entries = json.load(fid, cls=MontyDecoder) @@ -26,7 +26,7 @@ def setUp(self): c = ConversionElectrode.from_composition_and_entries( Composition(f), entries, working_ion_symbol=working_ion ) - self.conversion_eletrodes[f] = {"working_ion": working_ion, "CE": c} + self.conversion_electrodes[f] = {"working_ion": working_ion, "CE": c} self.expected_properties = { "LiCoO2": { @@ -68,7 +68,7 @@ def setUp(self): def test_init(self): # both 'LiCoO2' and "FeF3" are using Li+ as working ion; MnO2 is for the multivalent Mg2+ ion for f in self.formulas: - c = self.conversion_eletrodes[f]["CE"] + c = self.conversion_electrodes[f]["CE"] assert len(c.get_sub_electrodes(True)) == c.num_steps assert len(c.get_sub_electrodes(False)) == sum(range(1, c.num_steps + 1)) @@ -93,10 +93,19 @@ def test_init(self): for k, v in p.items(): assert getattr(electrode, "get_" + k)() == approx(v, abs=1e-2) + def test_repr(self): + conv_electrode = self.conversion_electrodes[self.formulas[0]]["CE"] + assert ( + repr(conv_electrode) + == "ConversionElectrode with formula='LiCoO2' and n_steps=5, avg_voltage=2.269 V, min_voltage=1.656 V, " + "max_voltage=3.492 V\nCapacity (grav.) 903.197 mAh/g, capacity (vol.) 2903.358 Ah/l\n" + "Specific energy 2049.719 Wh/kg, energy density 6588.890 Wh/l" + ) + def test_summary(self): kmap = {"specific_energy": "energy_grav", "energy_density": "energy_vol"} for f in self.formulas: - c = self.conversion_eletrodes[f]["CE"] + c = self.conversion_electrodes[f]["CE"] d = c.get_summary_dict() p = self.expected_properties[f] for k, v in p.items(): @@ -106,7 +115,7 @@ def test_summary(self): def test_composite(self): # check entries in charged/discharged state for formula in self.formulas: - CE = self.conversion_eletrodes[formula]["CE"] + CE = self.conversion_electrodes[formula]["CE"] for step, vpair in enumerate(CE.voltage_pairs): # entries_charge/entries_discharge attributes should return entries equal with the expected composite_dict = self.expected_composite[formula] diff --git a/tests/core/test_libxcfunc.py b/tests/core/test_libxcfunc.py index f26d4949eb2..435b71c389e 100644 --- a/tests/core/test_libxcfunc.py +++ b/tests/core/test_libxcfunc.py @@ -23,3 +23,8 @@ def test_libxcfunc_api(self): # Test if object supports MSONable self.assert_msonable(xc, test_is_subclass=False) + + def test_repr(self): + """Test LibxcFunc.__repr__""" + xc = LibxcFunc.LDA_C_HL + assert repr(xc) == "LibxcFunc(name='LDA_C_HL', kind='CORRELATION', family='LDA')" diff --git a/tests/entries/test_computed_entries.py b/tests/entries/test_computed_entries.py index 2eb59f8d9ce..801b46c3f67 100644 --- a/tests/entries/test_computed_entries.py +++ b/tests/entries/test_computed_entries.py @@ -43,7 +43,7 @@ def test_energy_adjustment_repr(): for cls, label in ((None, "unknown"), (comp_cls, cls_name), ({"@class": cls_name}, cls_name)): ea = EnergyAdjustment(10, cls=cls) assert ( - repr(ea) == "EnergyAdjustment(name='Manual adjustment', value=10.0, uncertainty=nan, description=, " + repr(ea) == "EnergyAdjustment(name='Manual adjustment', value=10.0, uncertainty=nan, description='', " f"generated_by='{label}')" ) diff --git a/tests/io/vasp/test_sets.py b/tests/io/vasp/test_sets.py index 79a9ca97436..2b34a946a34 100644 --- a/tests/io/vasp/test_sets.py +++ b/tests/io/vasp/test_sets.py @@ -589,7 +589,7 @@ def test_valid_magmom_struct(self): struct.insert(0, "Li", [0, 0, 0]) vis = MPRelaxSet(struct, user_potcar_settings={"Fe": "Fe"}, validate_magmom=False) - with pytest.raises(TypeError, match="argument must be a string or a real number, not .NoneType."): + with pytest.raises(TypeError, match="argument must be a string or a real number, not"): print(vis.get_vasp_input()) vis = MPRelaxSet(struct, user_potcar_settings={"Fe": "Fe"}, validate_magmom=True)