Deprecate CifParser.get_structures() in favor of new parse_structures in which primitive defaults to False (#3419)

* breaking: change default primitive=True to False in CifParser.get_structures()

* fix CifParser tests

* add test to ensure CIfParser.get_structures() and Structure.from_file() remain consistent in the future

* Structure.from_file add type hints and fix doc str claiming primitive keyword only passed to CIF

* add UserWarning for change of default primitive=True to False made on 2023-10-24

* deprecate get_structures() and change primitive default to False in new parse_structures() method

* use parse_structures in tests

* fix TestCifTransmuter.test_init

return self.parse_structures(*args, **kwargs)
E       TypeError: CifParser.parse_structures() got multiple values for argument 'primitive'

* add comment suggested by @JaGeo

* remove all internal use of deprecated CifParser.get_structures()

Author: janosh

pymatgen/alchemy/materials.py

@@ -269,7 +269,7 @@ def from_cif_str(
                 extracted. Defaults to True. However, there are certain
                 instances where you might want to use a non-primitive cell,
                 e.g., if you are trying to generate all possible orderings of
-                partial removals or order a disordered structure.
+                partial removals or order a disordered structure. Defaults to True.
             occupancy_tolerance (float): If total occupancy of a site is
                 between 1 and occupancy_tolerance, the occupancies will be
                 scaled down to 1.
@@ -281,7 +281,7 @@ def from_cif_str(
         raw_string = re.sub(r"'", '"', cif_string)
         cif_dict = parser.as_dict()
         cif_keys = list(cif_dict)
-        struct = parser.get_structures(primitive)[0]
+        struct = parser.parse_structures(primitive=primitive)[0]
         partial_cif = cif_dict[cif_keys[0]]
         if "_database_code_ICSD" in partial_cif:
             source = partial_cif["_database_code_ICSD"] + "-ICSD"

pymatgen/analysis/chemenv/utils/scripts_utils.py

@@ -240,7 +240,7 @@ def compute_environments(chemenv_configuration):
             if not found:
                 input_source = input("Enter path to CIF file : ")
             parser = CifParser(input_source)
-            structure = parser.get_structures()[0]
+            structure = parser.parse_structures(primitive=True)[0]
         elif source_type == "mp":
             if not found:
                 input_source = input('Enter materials project id (e.g. "mp-1902") : ')

pymatgen/core/structure.py

@@ -43,6 +43,7 @@
 
 if TYPE_CHECKING:
     from collections.abc import Iterable, Iterator, Sequence
+    from pathlib import Path
 
     from ase import Atoms
     from ase.calculators.calculator import Calculator
@@ -2767,7 +2768,7 @@ def from_str(
             from pymatgen.io.cif import CifParser
 
             parser = CifParser.from_str(input_string, **kwargs)
-            struct = parser.get_structures(primitive=primitive)[0]
+            struct = parser.parse_structures(primitive=primitive)[0]
         elif fmt_low == "poscar":
             from pymatgen.io.vasp import Poscar
 
@@ -2815,15 +2816,17 @@ def from_str(
         return cls.from_sites(struct, properties=struct.properties)
 
     @classmethod
-    def from_file(cls, filename, primitive=False, sort=False, merge_tol=0.0, **kwargs) -> Structure | IStructure:
+    def from_file(
+        cls, filename: str | Path, primitive: bool = False, sort: bool = False, merge_tol: float = 0.0, **kwargs
+    ) -> Structure | IStructure:
         """Reads a structure from a file. For example, anything ending in
         a "cif" is assumed to be a Crystallographic Information Format file.
         Supported formats include CIF, POSCAR/CONTCAR, CHGCAR, LOCPOT,
         vasprun.xml, CSSR, Netcdf and pymatgen's JSON-serialized structures.
 
         Args:
             filename (str): The filename to read from.
-            primitive (bool): Whether to convert to a primitive cell. Only available for CIFs. Defaults to False.
+            primitive (bool): Whether to convert to a primitive cell. Defaults to False.
             sort (bool): Whether to sort sites. Default to False.
             merge_tol (float): If this is some positive number, sites that are within merge_tol from each other will be
                 merged. Usually 0.01 should be enough to deal with common numerical issues.

pymatgen/io/cif.py

@@ -8,6 +8,7 @@
 import textwrap
 import warnings
 from collections import deque
+from datetime import datetime
 from functools import partial
 from inspect import getfullargspec as getargspec
 from io import StringIO
@@ -48,7 +49,7 @@ class CifBlock:
     attribute.
     """
 
-    maxlen = 70  # not quite 80 so we can deal with semicolons and things
+    max_len = 70  # not quite 80 so we can deal with semicolons and things
 
     def __init__(self, data, loops, header):
         """
@@ -89,7 +90,7 @@ def __str__(self):
             if key not in written:
                 # k didn't belong to a loop
                 v = self._format_field(self.data[key])
-                if len(key) + len(v) + 3 < self.maxlen:
+                if len(key) + len(v) + 3 < self.max_len:
                     out.append(f"{key}   {v}")
                 else:
                     out.extend([key, v])
@@ -105,7 +106,7 @@ def _loop_to_string(self, loop):
                 if val[0] == ";":
                     out += line + "\n" + val
                     line = "\n"
-                elif len(line) + len(val) + 2 < self.maxlen:
+                elif len(line) + len(val) + 2 < self.max_len:
                     line += "  " + val
                 else:
                     out += line
@@ -115,8 +116,8 @@ def _loop_to_string(self, loop):
 
     def _format_field(self, v):
         v = str(v).strip()
-        if len(v) > self.maxlen:
-            return ";\n" + textwrap.fill(v, self.maxlen) + "\n;"
+        if len(v) > self.max_len:
+            return ";\n" + textwrap.fill(v, self.max_len) + "\n;"
         # add quotes if necessary
         if v == "":
             return '""'
@@ -1132,18 +1133,35 @@ def get_matching_coord(coord):
             return struct
         return None
 
-    def get_structures(
+    @np.deprecate(
+        message="get_structures is deprecated and will be removed in 2024. Use parse_structures instead."
+        "The only difference is that primitive defaults to False in the new parse_structures method."
+        "So parse_structures(primitive=True) is equivalent to the old behavior of get_structures().",
+    )
+    def get_structures(self, *args, **kwargs) -> list[Structure]:
+        """
+        Deprecated. Use parse_structures instead. Only difference between the two methods is the
+        default primitive=False in parse_structures.
+        So parse_structures(primitive=True) is equivalent to the old behavior of get_structures().
+        """
+        if len(args) > 0:  # extract primitive if passed as arg
+            kwargs["primitive"] = args[0]
+            args = args[1:]
+        kwargs.setdefault("primitive", True)
+        return self.parse_structures(*args, **kwargs)
+
+    def parse_structures(
         self,
-        primitive: bool = True,
+        primitive: bool = False,
         symmetrized: bool = False,
         check_occu: bool = True,
         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.
-                Defaults to True. With magnetic CIF files, will return primitive
+            primitive (bool): Set to True to return primitive unit cells.
+                Defaults to False. With magnetic CIF files, True will return primitive
                 magnetic cell which may be larger than nuclear primitive cell.
             symmetrized (bool): If True, return a SymmetrizedStructure which will
                 include the equivalent indices and symmetry operations used to
@@ -1163,6 +1181,14 @@ def get_structures(
         Returns:
             list[Structure]: All structures in CIF file.
         """
+        if os.getenv("CI") and datetime.now() > datetime(2024, 3, 1):  # March 2024 seems long enough # pragma: no cover
+            raise RuntimeError("remove the change of default primitive=True to False made on 2023-10-24")
+        warnings.warn(
+            "The default value of primitive was changed from True to False in "
+            "https://github.com/materialsproject/pymatgen/pull/3419. CifParser now returns the cell "
+            "in the CIF file as is. If you want the primitive cell, please set primitive=True explicitly.",
+            UserWarning,
+        )
         if not check_occu:  # added in https://github.com/materialsproject/pymatgen/pull/2836
             warnings.warn("Structures with unphysical site occupancies are not compatible with many pymatgen features.")
         if primitive and symmetrized:
@@ -1326,18 +1352,18 @@ def __init__(
                 # primitive to conventional structures, the standard for CIF.
                 struct = sf.get_refined_structure()
 
-        latt = struct.lattice
+        lattice = struct.lattice
         comp = struct.composition
         no_oxi_comp = comp.element_composition
         block["_symmetry_space_group_name_H-M"] = spacegroup[0]
         for cell_attr in ["a", "b", "c"]:
-            block["_cell_length_" + cell_attr] = format_str.format(getattr(latt, cell_attr))
+            block["_cell_length_" + cell_attr] = format_str.format(getattr(lattice, cell_attr))
         for cell_attr in ["alpha", "beta", "gamma"]:
-            block["_cell_angle_" + cell_attr] = format_str.format(getattr(latt, cell_attr))
+            block["_cell_angle_" + cell_attr] = format_str.format(getattr(lattice, cell_attr))
         block["_symmetry_Int_Tables_number"] = spacegroup[1]
         block["_chemical_formula_structural"] = no_oxi_comp.reduced_formula
         block["_chemical_formula_sum"] = no_oxi_comp.formula
-        block["_cell_volume"] = format_str.format(latt.volume)
+        block["_cell_volume"] = format_str.format(lattice.volume)
 
         reduced_comp, fu = no_oxi_comp.get_reduced_composition_and_factor()
         block["_cell_formula_units_Z"] = str(int(fu))
@@ -1403,7 +1429,7 @@ def __init__(
 
                     magmom = Magmom(mag)
                     if write_magmoms and abs(magmom) > 0:
-                        moment = Magmom.get_moment_relative_to_crystal_axes(magmom, latt)
+                        moment = Magmom.get_moment_relative_to_crystal_axes(magmom, lattice)
                         atom_site_moment_label.append(f"{sp.symbol}{count}")
                         atom_site_moment_crystalaxis_x.append(format_str.format(moment[0]))
                         atom_site_moment_crystalaxis_y.append(format_str.format(moment[1]))

pymatgen/io/feff/inputs.py

@@ -207,8 +207,8 @@ def from_cif_file(cif_file, source="", comment=""):
         Returns:
             Header Object
         """
-        r = CifParser(cif_file)
-        structure = r.get_structures()[0]
+        parser = CifParser(cif_file)
+        structure = parser.parse_structures(primitive=True)[0]
         return Header(structure, source, comment)
 
     @property

tests/analysis/magnetism/test_analyzer.py

@@ -15,7 +15,6 @@
     magnetic_deformation,
 )
 from pymatgen.core import Element, Lattice, Species, Structure
-from pymatgen.io.cif import CifParser
 from pymatgen.util.testing import TEST_FILES_DIR
 
 enum_cmd = which("enum.x") or which("multienum.x")
@@ -25,45 +24,40 @@
 
 class TestCollinearMagneticStructureAnalyzer(unittest.TestCase):
     def setUp(self):
-        parser = CifParser(f"{TEST_FILES_DIR}/Fe.cif")
-        self.Fe = parser.get_structures()[0]
+        self.Fe = Structure.from_file(f"{TEST_FILES_DIR}/Fe.cif", primitive=True)
 
-        parser = CifParser(f"{TEST_FILES_DIR}/LiFePO4.cif")
-        self.LiFePO4 = parser.get_structures()[0]
+        self.LiFePO4 = Structure.from_file(f"{TEST_FILES_DIR}/LiFePO4.cif", primitive=True)
 
-        parser = CifParser(f"{TEST_FILES_DIR}/Fe3O4.cif")
-        self.Fe3O4 = parser.get_structures()[0]
+        self.Fe3O4 = Structure.from_file(f"{TEST_FILES_DIR}/Fe3O4.cif", primitive=True)
 
-        parser = CifParser(f"{TEST_FILES_DIR}/magnetic.ncl.example.GdB4.mcif")
-        self.GdB4 = parser.get_structures()[0]
+        self.GdB4 = Structure.from_file(f"{TEST_FILES_DIR}/magnetic.ncl.example.GdB4.mcif", primitive=True)
 
-        parser = CifParser(f"{TEST_FILES_DIR}/magnetic.example.NiO.mcif")
-        self.NiO_expt = parser.get_structures()[0]
+        self.NiO_expt = Structure.from_file(f"{TEST_FILES_DIR}/magnetic.example.NiO.mcif", primitive=True)
 
-        latt = Lattice.cubic(4.17)
+        lattice = Lattice.cubic(4.17)
         species = ["Ni", "O"]
         coords = [[0, 0, 0], [0.5, 0.5, 0.5]]
-        self.NiO = Structure.from_spacegroup(225, latt, species, coords)
+        self.NiO = Structure.from_spacegroup(225, lattice, species, coords)
 
-        latt = Lattice([[2.085, 2.085, 0.0], [0.0, -2.085, -2.085], [-2.085, 2.085, -4.17]])
+        lattice = Lattice([[2.085, 2.085, 0.0], [0.0, -2.085, -2.085], [-2.085, 2.085, -4.17]])
         species = ["Ni", "Ni", "O", "O"]
         coords = [[0.5, 0, 0.5], [0, 0, 0], [0.25, 0.5, 0.25], [0.75, 0.5, 0.75]]
-        self.NiO_AFM_111 = Structure(latt, species, coords, site_properties={"magmom": [-5, 5, 0, 0]})
+        self.NiO_AFM_111 = Structure(lattice, species, coords, site_properties={"magmom": [-5, 5, 0, 0]})
 
-        latt = Lattice([[2.085, 2.085, 0], [0, 0, -4.17], [-2.085, 2.085, 0]])
+        lattice = Lattice([[2.085, 2.085, 0], [0, 0, -4.17], [-2.085, 2.085, 0]])
         species = ["Ni", "Ni", "O", "O"]
         coords = [[0.5, 0.5, 0.5], [0, 0, 0], [0, 0.5, 0], [0.5, 0, 0.5]]
-        self.NiO_AFM_001 = Structure(latt, species, coords, site_properties={"magmom": [-5, 5, 0, 0]})
+        self.NiO_AFM_001 = Structure(lattice, species, coords, site_properties={"magmom": [-5, 5, 0, 0]})
 
-        latt = Lattice([[2.085, 2.085, 0], [0, 0, -4.17], [-2.085, 2.085, 0]])
+        lattice = Lattice([[2.085, 2.085, 0], [0, 0, -4.17], [-2.085, 2.085, 0]])
         species = ["Ni", "Ni", "O", "O"]
         coords = [[0.5, 0.5, 0.5], [0, 0, 0], [0, 0.5, 0], [0.5, 0, 0.5]]
-        self.NiO_AFM_001_opposite = Structure(latt, species, coords, site_properties={"magmom": [5, -5, 0, 0]})
+        self.NiO_AFM_001_opposite = Structure(lattice, species, coords, site_properties={"magmom": [5, -5, 0, 0]})
 
-        latt = Lattice([[2.085, 2.085, 0], [0, 0, -4.17], [-2.085, 2.085, 0]])
+        lattice = Lattice([[2.085, 2.085, 0], [0, 0, -4.17], [-2.085, 2.085, 0]])
         species = ["Ni", "Ni", "O", "O"]
         coords = [[0.5, 0.5, 0.5], [0, 0, 0], [0, 0.5, 0], [0.5, 0, 0.5]]
-        self.NiO_unphysical = Structure(latt, species, coords, site_properties={"magmom": [-3, 0, 0, 0]})
+        self.NiO_unphysical = Structure(lattice, species, coords, site_properties={"magmom": [-3, 0, 0, 0]})
 
     def test_get_representations(self):
         # tests to convert between storing magnetic moment information

tests/analysis/magnetism/test_jahnteller.py

@@ -6,7 +6,7 @@
 from pytest import approx
 
 from pymatgen.analysis.magnetism.jahnteller import JahnTellerAnalyzer, Species
-from pymatgen.io.cif import CifParser
+from pymatgen.core import Structure
 from pymatgen.util.testing import TEST_FILES_DIR
 
 
@@ -82,11 +82,9 @@ def test_jahn_teller_species_analysis(self):
         assert m == "none"
 
     def test_jahn_teller_structure_analysis(self):
-        parser = CifParser(f"{TEST_FILES_DIR}/LiFePO4.cif")
-        LiFePO4 = parser.get_structures()[0]
+        LiFePO4 = Structure.from_file(f"{TEST_FILES_DIR}/LiFePO4.cif", primitive=True)
 
-        parser = CifParser(f"{TEST_FILES_DIR}/Fe3O4.cif")
-        Fe3O4 = parser.get_structures()[0]
+        Fe3O4 = Structure.from_file(f"{TEST_FILES_DIR}/Fe3O4.cif", primitive=True)
 
         assert self.jt.is_jahn_teller_active(LiFePO4)
         assert self.jt.is_jahn_teller_active(Fe3O4)

tests/core/test_structure.py

@@ -26,6 +26,7 @@
 )
 from pymatgen.electronic_structure.core import Magmom
 from pymatgen.io.ase import AseAtomsAdaptor
+from pymatgen.io.cif import CifParser
 from pymatgen.symmetry.analyzer import SpacegroupAnalyzer
 from pymatgen.util.testing import TEST_FILES_DIR, PymatgenTest
 
@@ -778,6 +779,10 @@ def test_to_from_file_string(self):
         struct = Structure.from_file(f"{TEST_FILES_DIR}/bad-unicode-gh-2947.mcif")
         assert struct.formula == "Ni32 O32"
 
+        # make sure CIfParser.parse_structures() and Structure.from_file() are consistent
+        # i.e. uses same merge_tol for site merging, same primitive=False, etc.
+        assert struct == CifParser(f"{TEST_FILES_DIR}/bad-unicode-gh-2947.mcif").parse_structures()[0]
+
     def test_to_file_alias(self):
         out_path = f"{self.tmp_path}/POSCAR"
         assert self.struct.to(out_path) == self.struct.to_file(out_path)

tests/ext/test_matproj.py

@@ -21,7 +21,6 @@
 from pymatgen.entries.computed_entries import ComputedEntry
 from pymatgen.ext.matproj import MP_LOG_FILE, MPRestError, _MPResterBasic
 from pymatgen.ext.matproj_legacy import TaskType, _MPResterLegacy
-from pymatgen.io.cif import CifParser
 from pymatgen.phonon.bandstructure import PhononBandStructureSymmLine
 from pymatgen.phonon.dos import CompletePhononDos
 from pymatgen.util.testing import TEST_FILES_DIR, PymatgenTest
@@ -127,8 +126,8 @@ def test_find_structure(self):
         cif_file = f"{TEST_FILES_DIR}/Fe3O4.cif"
         data = mpr.find_structure(str(cif_file))
         assert len(data) > 1
-        s = CifParser(cif_file).get_structures()[0]
-        data = mpr.find_structure(s)
+        struct = Structure.from_file(cif_file, primitive=True)
+        data = mpr.find_structure(struct)
         assert len(data) > 1
 
     def test_get_entries_in_chemsys(self):
@@ -622,15 +621,6 @@ def test_get_all_materials_ids_doc(self):
     #     data = mpr.get_materials_id_references("mp-123")
     #     assert len(data) > 1000
 
-    # def test_find_structure(self):
-    #     mpr = _MPResterLegacy()
-    #     cif_file = f"{TEST_FILES_DIR}/Fe3O4.cif"
-    #     data = mpr.find_structure(str(cif_file))
-    #     assert len(data) > 1
-    #     s = CifParser(cif_file).get_structures()[0]
-    #     data = mpr.find_structure(s)
-    #     assert len(data) > 1
-
     def test_get_entries_and_in_chemsys(self):
         # One large system test.
         syms = ["Li", "Fe", "O", "P", "Mn"]

tests/io/feff/test_inputs.py

@@ -7,7 +7,6 @@
 from pytest import approx
 
 from pymatgen.core import Molecule, Structure
-from pymatgen.io.cif import CifParser
 from pymatgen.io.feff.inputs import Atoms, Header, Paths, Potential, Tags
 from pymatgen.util.testing import TEST_FILES_DIR
 
@@ -59,8 +58,7 @@ def test_as_dict_and_from_dict(self):
 class TestFeffAtoms(unittest.TestCase):
     @classmethod
     def setUpClass(cls):
-        r = CifParser(f"{TEST_FILES_DIR}/CoO19128.cif")
-        cls.structure = r.get_structures()[0]
+        cls.structure = Structure.from_file(f"{TEST_FILES_DIR}/CoO19128.cif")
         cls.atoms = Atoms(cls.structure, "O", 12.0)
 
     def test_absorbing_atom(self):