Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking: Update AseAtomsAdaptor to handle Structure.properties/Molecule.properties #3270

Merged
merged 15 commits into from
Aug 31, 2023
Merged

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Aug 24, 2023

Ensure Structure.properties and Molecule.properties do not get lost with ASE Atoms interconversion.

Recent PR that added properties: #3264

Breaking because IMolecule.__eq__ and IStructure.__eq__ were changed to check self.properties == other.properties. See #3270 (comment).

@Andrew-S-Rosen Andrew-S-Rosen changed the title Ensure Structure.properties and Molecule.properties do not get lost with ASE Atoms interconversion Update AseAtomsAdaptor to handle Structure.properties/Molecule.properties Aug 24, 2023
@Andrew-S-Rosen Andrew-S-Rosen marked this pull request as draft August 24, 2023 19:38
@janosh janosh added enhancement A new feature or improvement to an existing one ase Atomic simulation environment labels Aug 24, 2023
@Andrew-S-Rosen
Copy link
Member Author

I see you @shyuep! 👀😅

I plan to finish this PR later today.

@Andrew-S-Rosen Andrew-S-Rosen marked this pull request as ready for review August 31, 2023 01:42
@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Aug 31, 2023

@janosh: When you're back on GitHub, would you mind helping me out with the mypy issue? I think it might be because .properties is not a @property attribute of Structure or Molecule, but I'm not 100% sure. Thank you 🙏 Not urgent!

@Andrew-S-Rosen Andrew-S-Rosen changed the title Update AseAtomsAdaptor to handle Structure.properties/Molecule.properties [WIP] Update AseAtomsAdaptor to handle Structure.properties/Molecule.properties Aug 31, 2023
@Andrew-S-Rosen Andrew-S-Rosen changed the title [WIP] Update AseAtomsAdaptor to handle Structure.properties/Molecule.properties Update AseAtomsAdaptor to handle Structure.properties/Molecule.properties Aug 31, 2023
@@ -292,10 +297,14 @@ def test_back_forth(self):
# Molecule --> Atoms --> Molecule --> Atoms
molecule = Molecule.from_file(TEST_FILES_DIR / "acetylene.xyz")
molecule.set_charge_and_spin(-2, spin_multiplicity=3)
molecule.info = {"test": "hi"}
molecule.properties = {"test": "hi"}
atoms = aio.AseAtomsAdaptor.get_atoms(molecule)
molecule_back = aio.AseAtomsAdaptor.get_molecule(atoms)
atoms_back = aio.AseAtomsAdaptor.get_atoms(molecule_back)
for k, v in atoms.todict().items():
assert str(atoms_back.todict()[k]) == str(v)
assert molecule_back == molecule
Copy link
Member

@janosh janosh Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't check that the properties were restored since Molecule.__eq__ doesn't currently look at properties. I think that'll be unexpected behavior for must users so we should change IMolecule.__eq__ and IStructure.__eq__ even though it's strictly speaking a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that that's worthwhile to change!

@janosh janosh changed the title Update AseAtomsAdaptor to handle Structure.properties/Molecule.properties Breaking: Update AseAtomsAdaptor to handle Structure.properties/Molecule.properties Aug 31, 2023
@janosh janosh added the breaking Breaking change label Aug 31, 2023
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Thanks @arosen93! 👍

@janosh janosh merged commit 50b4af3 into materialsproject:master Aug 31, 2023
@@ -216,12 +217,19 @@ def get_structure(atoms: Atoms, cls: type[Structure] = Structure, **cls_kwargs)
else:
sel_dyn = None

# Atoms.info <---> Structure.properties (excluding properties["calc"])
properties = jsanitize(getattr(atoms, "info", {}))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janosh: Just wanted to point out one subtlety to you. Currently, we have the mapping: Atoms.info <--> Structure.properties, since they serve the same purpose between ASE and Pymtatgen, respectively. However, I jsanitize-d the atoms.info before assigning it to .properties to ensure the Structure/Molecule object can be (de)serialized since this is often an implicit assumption of Structure/Molecule objects but is not necessarily one for ASE Atoms objects. In practice, this means that there could be some scenarios where a user interconverts between ASE Atoms and Pymatgen Structure/Molecule with some loss of information; namely, if the atoms.info is modified by jsanitize() then it can't be restored in its original form. There might be a better solution than this, but I implemented it because oftentimes ASE will dump un-serializable class objects (namely, a Spacegroup class) into atoms.info, such as when reading in a CIF with symmetry flags.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for highlighting! Seems like a good way to go about this for now. I'll keep this in mind if we run into any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ase Atomic simulation environment breaking Breaking change enhancement A new feature or improvement to an existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants