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

Any thoughts on how to handle non-serializable Structure.properties? #3297

Closed
Andrew-S-Rosen opened this issue Sep 2, 2023 · 1 comment
Closed

Comments

@Andrew-S-Rosen
Copy link
Member

Andrew-S-Rosen commented Sep 2, 2023

Recently, a .properties attribute was added to the Structure/Molecule objects in #3264. However, since the user can provide any data they want here, it has the potential to break serialization (same thing with the pre-existing .site_properties, technically, but .properties is more open-ended).

We should think about how we best want to handle this (e.g. do we raise a warning to the user, do nothing at all, sanitize the data, something else?). It's possible that "do nothing at all" is the best approach, as the user will get an error when they try to dump the object if there is something non-serializable in it, but I wanted to open this up for broader discussion anyway.


On a related note, in the ASEAtomsAdapter, I have the following line:

properties = jsanitize(getattr(atoms, "info", {}))

The jsanitize part was added because oftentimes the atoms.info dictionary contains an entry that is not serializable, like the ASE Spacegroup object when you read in a CIF. By using jsanitize, it ensures that things are always clean. However, it makes me slightly uncomfortable because if someone converts back and forth between Atoms and Structure/Molecule, it's possible that the starting object is not the same due to jsanitize being called on some aspect of the data. Given how often there is non-serializable data in atoms.info, I felt it necessary to not simply ignore it, but I am not convinced that the current approach is ideal and am open to suggestions.

@Andrew-S-Rosen Andrew-S-Rosen changed the title Think about how to better handle (de)serialization with .properties Think about how to handle non-serializable Structure.properties Sep 2, 2023
@Andrew-S-Rosen Andrew-S-Rosen changed the title Think about how to handle non-serializable Structure.properties Any thoughts on how to handle non-serializable Structure.properties? Sep 3, 2023
@shyuep
Copy link
Member

shyuep commented Sep 5, 2023

I would assume properties must be something that can be handled by MontyEncoder. This can be made explicit in the doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants