From 3d477472f93b4ee4ad9cc47fe827a59b9e3d912f Mon Sep 17 00:00:00 2001 From: wolearyc Date: Wed, 19 Feb 2025 11:56:15 -0800 Subject: [PATCH 1/5] deepcopy atoms.info/structure.properties during ase/pymatgen conversion this fixes bug where atoms/structure objects were themselves modified when converted --- src/pymatgen/io/ase.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/pymatgen/io/ase.py b/src/pymatgen/io/ase.py index 308128c798b..d10303af092 100644 --- a/src/pymatgen/io/ase.py +++ b/src/pymatgen/io/ase.py @@ -8,6 +8,7 @@ import warnings from importlib.metadata import PackageNotFoundError from typing import TYPE_CHECKING +import copy import numpy as np from monty.json import MontyDecoder, MSONable, jsanitize @@ -204,7 +205,7 @@ def get_atoms(structure: SiteCollection, msonable: bool = True, **kwargs) -> MSO # Atoms.info <---> Structure.properties if properties := structure.properties: - atoms.info = properties + atoms.info = copy.deepcopy(properties) # Regenerate Spacegroup object from `.todict()` representation if isinstance(atoms.info.get("spacegroup"), dict): @@ -298,10 +299,11 @@ def get_structure(atoms: Atoms, cls: type[Structure] = Structure, **cls_kwargs) sel_dyn = None # Atoms.info <---> Structure.properties - # But first make sure `spacegroup` is JSON serializable - if atoms.info.get("spacegroup") and isinstance(atoms.info["spacegroup"], Spacegroup): - atoms.info["spacegroup"] = atoms.info["spacegroup"].todict() - properties = getattr(atoms, "info", {}) + properties = copy.deepcopy(getattr(atoms, "info", {})) + # If present, convert Spacegroup object to JSON-serializable dict + if properties.get("spacegroup") and isinstance(properties["spacegroup"], Spacegroup): + properties["spacegroup"] = properties["spacegroup"].todict() + # Return a Molecule object if that was specifically requested; # otherwise return a Structure object as expected From 1730f29eb617f15ce13a8977104d18c3dfb871bb Mon Sep 17 00:00:00 2001 From: wolearyc Date: Wed, 19 Feb 2025 11:58:06 -0800 Subject: [PATCH 2/5] added tests to check spacegroup conversions --- tests/io/test_ase.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/io/test_ase.py b/tests/io/test_ase.py index 4328d0f6959..5c74445c4ef 100644 --- a/tests/io/test_ase.py +++ b/tests/io/test_ase.py @@ -104,6 +104,21 @@ def test_get_atoms_from_structure_dyn(): ase_mask = c.mask if isinstance(c, ase.constraints.FixCartesian) else [True, True, True] assert len(c.index) == len([mask for mask in sel_dyn if np.array_equal(mask, ~np.array(ase_mask))]) +def test_get_atoms_from_structure_spacegroup(): + # Get structure with space group dictionary in properties + space_group_info = STRUCTURE.get_space_group_info() + STRUCTURE.properties["spacegroup"] = {"number": space_group_info[1], "setting": 1} + + # Convert to atoms and check that structure was not modified + atoms = AseAtomsAdaptor.get_atoms(STRUCTURE) + assert isinstance(STRUCTURE.properties["spacegroup"], dict) + assert isinstance(atoms.info["spacegroup"], ase.spacegroup.Spacegroup) + + # Check that space group matches + assert atoms.info["spacegroup"].no == STRUCTURE.properties["spacegroup"]["number"] + assert atoms.info["spacegroup"].setting == STRUCTURE.properties["spacegroup"]["setting"] + + def test_get_atoms_from_molecule(): mol = Molecule.from_file(XYZ_STRUCTURE) @@ -216,6 +231,24 @@ def test_get_structure_dyn(select_dyn): assert len(ase_atoms) == len(structure) +def test_get_structure_spacegroup(): + # set up Atoms object with spacegroup information + a = 4.05 + atoms = ase.spacegroup.crystal( + "Al", [(0, 0, 0)], spacegroup=225, cellpar=[a, a, a, 90, 90, 90] + ) + assert "spacegroup" in atoms.info + assert isinstance(atoms.info["spacegroup"], ase.spacegroup.Spacegroup) + + # Test that get_structure does not mutate atoms + structure = AseAtomsAdaptor.get_structure(atoms) + assert isinstance(atoms.info["spacegroup"], ase.spacegroup.Spacegroup) + assert isinstance(structure.properties["spacegroup"], dict) + + # Test that spacegroup info matches + assert atoms.info["spacegroup"].no == structure.properties["spacegroup"]["number"] + assert atoms.info["spacegroup"].setting == structure.properties["spacegroup"]["setting"] + def test_get_molecule(): atoms = ase.io.read(XYZ_STRUCTURE) From d4375b00c465e5c157620a43b15751df83dd7de1 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 19 Feb 2025 20:12:13 +0000 Subject: [PATCH 3/5] pre-commit auto-fixes --- src/pymatgen/io/ase.py | 3 +-- tests/io/test_ase.py | 9 ++++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/pymatgen/io/ase.py b/src/pymatgen/io/ase.py index d10303af092..ed02e2d8e30 100644 --- a/src/pymatgen/io/ase.py +++ b/src/pymatgen/io/ase.py @@ -5,10 +5,10 @@ from __future__ import annotations +import copy import warnings from importlib.metadata import PackageNotFoundError from typing import TYPE_CHECKING -import copy import numpy as np from monty.json import MontyDecoder, MSONable, jsanitize @@ -303,7 +303,6 @@ def get_structure(atoms: Atoms, cls: type[Structure] = Structure, **cls_kwargs) # If present, convert Spacegroup object to JSON-serializable dict if properties.get("spacegroup") and isinstance(properties["spacegroup"], Spacegroup): properties["spacegroup"] = properties["spacegroup"].todict() - # Return a Molecule object if that was specifically requested; # otherwise return a Structure object as expected diff --git a/tests/io/test_ase.py b/tests/io/test_ase.py index 5c74445c4ef..87e101cef36 100644 --- a/tests/io/test_ase.py +++ b/tests/io/test_ase.py @@ -104,6 +104,7 @@ def test_get_atoms_from_structure_dyn(): ase_mask = c.mask if isinstance(c, ase.constraints.FixCartesian) else [True, True, True] assert len(c.index) == len([mask for mask in sel_dyn if np.array_equal(mask, ~np.array(ase_mask))]) + def test_get_atoms_from_structure_spacegroup(): # Get structure with space group dictionary in properties space_group_info = STRUCTURE.get_space_group_info() @@ -119,7 +120,6 @@ def test_get_atoms_from_structure_spacegroup(): assert atoms.info["spacegroup"].setting == STRUCTURE.properties["spacegroup"]["setting"] - def test_get_atoms_from_molecule(): mol = Molecule.from_file(XYZ_STRUCTURE) atoms = AseAtomsAdaptor.get_atoms(mol) @@ -231,18 +231,17 @@ def test_get_structure_dyn(select_dyn): assert len(ase_atoms) == len(structure) + def test_get_structure_spacegroup(): # set up Atoms object with spacegroup information a = 4.05 - atoms = ase.spacegroup.crystal( - "Al", [(0, 0, 0)], spacegroup=225, cellpar=[a, a, a, 90, 90, 90] - ) + atoms = ase.spacegroup.crystal("Al", [(0, 0, 0)], spacegroup=225, cellpar=[a, a, a, 90, 90, 90]) assert "spacegroup" in atoms.info assert isinstance(atoms.info["spacegroup"], ase.spacegroup.Spacegroup) # Test that get_structure does not mutate atoms structure = AseAtomsAdaptor.get_structure(atoms) - assert isinstance(atoms.info["spacegroup"], ase.spacegroup.Spacegroup) + assert isinstance(atoms.info["spacegroup"], ase.spacegroup.Spacegroup) assert isinstance(structure.properties["spacegroup"], dict) # Test that spacegroup info matches From ec003c910dc3547c08b09a16600d5cda5f506614 Mon Sep 17 00:00:00 2001 From: wolearyc Date: Thu, 20 Feb 2025 07:55:40 -0800 Subject: [PATCH 4/5] selective import for copy.deepcopy in keeping with style of ase.py --- src/pymatgen/io/ase.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pymatgen/io/ase.py b/src/pymatgen/io/ase.py index ed02e2d8e30..b19f748eba7 100644 --- a/src/pymatgen/io/ase.py +++ b/src/pymatgen/io/ase.py @@ -5,7 +5,7 @@ from __future__ import annotations -import copy +from copy import deepcopy import warnings from importlib.metadata import PackageNotFoundError from typing import TYPE_CHECKING @@ -205,7 +205,7 @@ def get_atoms(structure: SiteCollection, msonable: bool = True, **kwargs) -> MSO # Atoms.info <---> Structure.properties if properties := structure.properties: - atoms.info = copy.deepcopy(properties) + atoms.info = deepcopy(properties) # Regenerate Spacegroup object from `.todict()` representation if isinstance(atoms.info.get("spacegroup"), dict): @@ -299,7 +299,7 @@ def get_structure(atoms: Atoms, cls: type[Structure] = Structure, **cls_kwargs) sel_dyn = None # Atoms.info <---> Structure.properties - properties = copy.deepcopy(getattr(atoms, "info", {})) + properties = deepcopy(getattr(atoms, "info", {})) # If present, convert Spacegroup object to JSON-serializable dict if properties.get("spacegroup") and isinstance(properties["spacegroup"], Spacegroup): properties["spacegroup"] = properties["spacegroup"].todict() From f01487cb674b96ac1f596b95745cf38c2e4bca24 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 20 Feb 2025 16:00:33 +0000 Subject: [PATCH 5/5] pre-commit auto-fixes --- src/pymatgen/io/ase.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pymatgen/io/ase.py b/src/pymatgen/io/ase.py index b19f748eba7..bbd0e2bef0c 100644 --- a/src/pymatgen/io/ase.py +++ b/src/pymatgen/io/ase.py @@ -5,8 +5,8 @@ from __future__ import annotations -from copy import deepcopy import warnings +from copy import deepcopy from importlib.metadata import PackageNotFoundError from typing import TYPE_CHECKING