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

Don't rely on jsanitize in Atoms <--> Structure object interconversion #3359

Merged
merged 25 commits into from
Sep 28, 2023
Merged

Don't rely on jsanitize in Atoms <--> Structure object interconversion #3359

merged 25 commits into from
Sep 28, 2023

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Sep 28, 2023

Closes #3358.

As raised in #3270, I stated:

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.

I now have avoided the needed to call jsanitize so that we don't lose any info between interconversion. To do this, I have "manually" JSON-ified and reconstructed the ASE Spacegroup object if present. I also did a much-needed general cleanup of both the class and test suite.

@Andrew-S-Rosen Andrew-S-Rosen changed the title Don't rely on jsanitize in Atoms <--> Structure object interconversion [WIP] Don't rely on jsanitize in Atoms <--> Structure object interconversion Sep 28, 2023
@Andrew-S-Rosen Andrew-S-Rosen changed the title [WIP] Don't rely on jsanitize in Atoms <--> Structure object interconversion Don't rely on jsanitize in Atoms <--> Structure object interconversion Sep 28, 2023
@Andrew-S-Rosen
Copy link
Member Author

@chiang-yuan, @janosh: This is ready for your review.

@shyuep shyuep merged commit f758c60 into materialsproject:master Sep 28, 2023
16 of 18 checks passed
@chiang-yuan
Copy link
Contributor

Just want to offer thanks for this! @Andrew-S-Rosen you are so swift on this! (even on PTO 🫡

@Andrew-S-Rosen
Copy link
Member Author

Had a spare moment and couldn't get it off my mind. I can go back to the beach life now. 😎🏖️

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

Successfully merging this pull request may close these issues.

jsanitize has downstream issue when converting ase Atoms to pymatgen Structure in multiprocessing
4 participants