From 92d00cdcfd5cca3cbd5e9f6ee84aace63fe9bde6 Mon Sep 17 00:00:00 2001 From: Janosh Riebesell Date: Wed, 11 Oct 2023 11:44:45 -0700 Subject: [PATCH] add TODO comment re overwriting blocks with equal headers in CifFile.from_str() --- pymatgen/apps/battery/analyzer.py | 12 +++++------ pymatgen/io/cif.py | 4 ++-- .../advanced_transformations.py | 12 +++++------ tests/apps/battery/test_analyzer.py | 8 +++++--- tests/files/.pytest-split-durations | 4 ++-- tests/io/test_atat.py | 20 +++++++++---------- 6 files changed, 31 insertions(+), 29 deletions(-) diff --git a/pymatgen/apps/battery/analyzer.py b/pymatgen/apps/battery/analyzer.py index 93c177063d0..5408399846c 100644 --- a/pymatgen/apps/battery/analyzer.py +++ b/pymatgen/apps/battery/analyzer.py @@ -25,22 +25,22 @@ class BatteryAnalyzer: """A suite of methods for starting with an oxidized structure and determining its potential as a battery.""" - def __init__(self, struc_oxid, working_ion="Li", oxi_override=None): + def __init__(self, struct_oxid, working_ion="Li", oxi_override=None): """Pass in a structure for analysis. Arguments: - struc_oxid: a Structure object; oxidation states *must* be assigned for this structure; disordered + struct_oxid: a Structure object; oxidation states *must* be assigned for this structure; disordered structures should be OK working_ion: a String symbol or Element for the working ion. oxi_override: a dict of String element symbol, Integer oxidation state pairs. by default, H, C, N, O, F, S, Cl, Se, Br, Te, I are considered anions. """ - for site in struc_oxid: + for site in struct_oxid: if not hasattr(site.specie, "oxi_state"): raise ValueError("BatteryAnalyzer requires oxidation states assigned to structure!") - self.struc_oxid = struc_oxid + self.struct_oxid = struct_oxid self.oxi_override = oxi_override or {} - self.comp = self.struc_oxid.composition # shortcut for later + self.comp = self.struct_oxid.composition # shortcut for later if not isinstance(working_ion, Element): self.working_ion = Element(working_ion) @@ -152,7 +152,7 @@ def get_max_capvol(self, remove=True, insert=True, volume=None): Returns: max vol capacity in mAh/cc """ - vol = volume or self.struc_oxid.volume + vol = volume or self.struct_oxid.volume return self._get_max_cap_ah(remove, insert) * 1000 * 1e24 / (vol * const.N_A) def get_removals_int_oxid(self): diff --git a/pymatgen/io/cif.py b/pymatgen/io/cif.py index a8b0481375c..19123e5279b 100644 --- a/pymatgen/io/cif.py +++ b/pymatgen/io/cif.py @@ -265,6 +265,8 @@ def from_str(cls, string) -> CifFile: if "powder_pattern" in re.split(r"\n", block_str, maxsplit=1)[0]: continue block = CifBlock.from_str("data_" + block_str) + # TODO (@janosh, 2023-10-11) multiple CIF blocks with equal header will overwrite each other, + # latest taking precedence. maybe something to fix and test e.g. in test_cif_writer_write_file dct[block.header] = block return cls(dct, string) @@ -1493,8 +1495,6 @@ def write_file(self, filename: str | Path, mode: Literal["w", "a", "wt", "at"] = """Write the CIF file.""" with zopen(filename, mode=mode) as file: file.write(str(self)) - if mode in ["a", "at"]: - file.write("\n\n") def str2float(text): diff --git a/pymatgen/transformations/advanced_transformations.py b/pymatgen/transformations/advanced_transformations.py index b4daf02cbde..f015811aa9c 100644 --- a/pymatgen/transformations/advanced_transformations.py +++ b/pymatgen/transformations/advanced_transformations.py @@ -1962,16 +1962,16 @@ def _get_max_neighbor_distance(struct, shell): return max(distances) @staticmethod - def _get_disordered_substructure(struc_disordered): + def _get_disordered_substructure(struct_disordered): """Converts disordered structure into a substructure consisting of only disordered sites. Args: - struc_disordered: pymatgen disordered Structure object. + struct_disordered: pymatgen disordered Structure object. Returns: pymatgen Structure object representing a substructure of disordered sites. """ - disordered_substructure = struc_disordered.copy() + disordered_substructure = struct_disordered.copy() idx_to_remove = [idx for idx, site in enumerate(disordered_substructure) if site.is_ordered] disordered_substructure.remove_sites(idx_to_remove) @@ -1979,11 +1979,11 @@ def _get_disordered_substructure(struc_disordered): return disordered_substructure @staticmethod - def _sqs_cluster_estimate(struc_disordered, cluster_size_and_shell: dict[int, int] | None = None): + def _sqs_cluster_estimate(struct_disordered, cluster_size_and_shell: dict[int, int] | None = None): """Set up an ATAT cluster.out file for a given structure and set of constraints. Args: - struc_disordered: disordered pymatgen Structure object + struct_disordered: disordered pymatgen Structure object cluster_size_and_shell: dict of integers {cluster: shell}. Returns: @@ -1991,7 +1991,7 @@ def _sqs_cluster_estimate(struc_disordered, cluster_size_and_shell: dict[int, in """ cluster_size_and_shell = cluster_size_and_shell or {2: 3, 3: 2, 4: 1} - disordered_substructure = SQSTransformation._get_disordered_substructure(struc_disordered) + disordered_substructure = SQSTransformation._get_disordered_substructure(struct_disordered) clusters = {} for cluster_size, shell in cluster_size_and_shell.items(): diff --git a/tests/apps/battery/test_analyzer.py b/tests/apps/battery/test_analyzer.py index 184fea1b1b1..cd833d91583 100644 --- a/tests/apps/battery/test_analyzer.py +++ b/tests/apps/battery/test_analyzer.py @@ -39,7 +39,7 @@ def test_oxide_check(self): with pytest.raises(ValueError, match="BatteryAnalyzer requires oxidation states assigned to structure"): BatteryAnalyzer(struct, "Li") - def test_capacitygrav_calculations(self): + def test_capacity_grav_calculations(self): li_fe_p_o4_cap = 169.89053 # same as fe_po4 cap na_fe_p_o4_cap = 154.20331 la2_co_o4_f_cap = 175.6564 @@ -63,7 +63,7 @@ def test_capacitygrav_calculations(self): assert self.li3v2p3o12.get_max_capgrav(insert=False) == approx(li3_v2_p3_o12_cap_remove, abs=1e-3) assert self.li3v2p3o12.get_max_capgrav(remove=False) == approx(li3_v2_p3_o12_cap_insert, abs=1e-3) - def test_capacityvol_calculations(self): + def test_capacity_vol_calculations(self): li_fe_p_o4_cap = 594.17518 na_fe_p_o4_cap = 542.86104 @@ -82,7 +82,9 @@ def test_capacityvol_calculations(self): assert self.fe_p_o4.get_max_capvol(insert=False) == 0 # give the lifepo4 volume, should get lifepo4 capacity - assert self.fe_p_o4.get_max_capvol(volume=self.li_fe_p_o4.struc_oxid.volume) == approx(li_fe_p_o4_cap, abs=1e-3) + assert self.fe_p_o4.get_max_capvol(volume=self.li_fe_p_o4.struct_oxid.volume) == approx( + li_fe_p_o4_cap, abs=1e-3 + ) def test_ion_removal(self): assert self.lifemnpo4.get_removals_int_oxid() == {1, 2, 3, 4} diff --git a/tests/files/.pytest-split-durations b/tests/files/.pytest-split-durations index d454701cfd6..6e407e52540 100644 --- a/tests/files/.pytest-split-durations +++ b/tests/files/.pytest-split-durations @@ -672,8 +672,8 @@ "tests/analysis/xas/test_spectrum.py::TestXAS::test_str": 0.002054751035757363, "tests/analysis/xas/test_spectrum.py::TestXAS::test_as_from_dict": 0.002419874945189804, "tests/analysis/xas/test_spectrum.py::TestXAS::test_validate": 0.0020956239895895123, - "tests/apps/battery/test_analyzer.py::TestBatteryAnalyzer::test_capacitygrav_calculations": 0.015028917114250362, - "tests/apps/battery/test_analyzer.py::TestBatteryAnalyzer::test_capacityvol_calculations": 0.011784416041336954, + "tests/apps/battery/test_analyzer.py::TestBatteryAnalyzer::test_capacity_grav_calculations": 0.015028917114250362, + "tests/apps/battery/test_analyzer.py::TestBatteryAnalyzer::test_capacity_vol_calculations": 0.011784416041336954, "tests/apps/battery/test_analyzer.py::TestBatteryAnalyzer::test_ion_removal": 0.052120123989880085, "tests/apps/battery/test_analyzer.py::TestBatteryAnalyzer::test_oxide_check": 0.011619041964877397, "tests/apps/battery/test_conversion_battery.py::TestConversionElectrode::test_composite": 0.02429362601833418, diff --git a/tests/io/test_atat.py b/tests/io/test_atat.py index 42d8ac43a38..41aa4c9a420 100644 --- a/tests/io/test_atat.py +++ b/tests/io/test_atat.py @@ -87,28 +87,28 @@ def test_mcsqs_export(self): def test_mcsqs_cif_nacl(self): # CIF file from str2cif (utility distributed with atat) - struc_from_cif = Structure.from_file(f"{test_dir}/bestsqs_nacl.cif") + struct_from_cif = Structure.from_file(f"{test_dir}/bestsqs_nacl.cif") # output file directly from mcsqs - struc_from_out = Structure.from_file(f"{test_dir}/bestsqs_nacl.out") + struct_from_out = Structure.from_file(f"{test_dir}/bestsqs_nacl.out") - assert struc_from_cif.matches(struc_from_out) + assert struct_from_cif.matches(struct_from_out) assert_allclose( - struc_from_out.lattice.parameters, - struc_from_cif.lattice.parameters, + struct_from_out.lattice.parameters, + struct_from_cif.lattice.parameters, atol=1e-4, ) def test_mcsqs_cif_pzt(self): # CIF file from str2cif (utility distributed with atat) - struc_from_cif = Structure.from_file(f"{test_dir}/bestsqs_pzt.cif") + struct_from_cif = Structure.from_file(f"{test_dir}/bestsqs_pzt.cif") # output file directly from mcsqs - struc_from_out = Structure.from_file(f"{test_dir}/bestsqs_pzt.out") + struct_from_out = Structure.from_file(f"{test_dir}/bestsqs_pzt.out") - assert struc_from_cif.matches(struc_from_out) + assert struct_from_cif.matches(struct_from_out) assert_allclose( - struc_from_out.lattice.parameters, - struc_from_cif.lattice.parameters, + struct_from_out.lattice.parameters, + struct_from_cif.lattice.parameters, atol=1e-4, )