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

Add properties to Structure and Molecule #3264

Merged
merged 17 commits into from
Aug 24, 2023

Conversation

gpetretto
Copy link
Contributor

@gpetretto gpetretto commented Aug 23, 2023

Closes #2884.

This PR implements a properties attribute for Structure and Molecule. I also modified the Poscar object to store the predictor_corrector_preamble in the structure properties.

@janosh
Copy link
Member

janosh commented Aug 23, 2023

Thanks @gpetretto for taking this on! Looks promising. 👍

I think we need better file IO support before merging. Right now, the properties are written to JSON

json_str = struct_with_props.to(fmt="json")
assert '"test_property": 42' in json_str

but they're not read back in Structure.from_str, i.e. this fails:

struct = Structure.from_str(json_str, fmt="json")
assert struct.properties == props

As I mentioned in #2884 (comment) it would be nice to retain properties when reading and writing to as many file formats as possible. JSON at least is easy to do.

@janosh
Copy link
Member

janosh commented Aug 23, 2023

@arosen93 @utf Would you mind taking a look at this to ensure it addresses your use cases?

@Andrew-S-Rosen
Copy link
Member

At a very quick glance, this seems reasonable. Once merged, I will need to update the ASE atoms adapter to ensure this is handled nicely. But that's for me to deal with.

@janosh janosh merged commit f897eaa into materialsproject:master Aug 24, 2023
@janosh
Copy link
Member

janosh commented Aug 24, 2023

Thanks @gpetretto! 👍

@Andrew-S-Rosen
Copy link
Member

@gpetretto: Any thoughts about (de)serialization with the new .properties flag? In principle, a user can define .properties such that it contains data that is not jsonable or that makes the following fail. Is this something where we put the onus on the user, or should we be incorporating some sort of check?

d = jsanitize(structure, strict=True, enum_values=True)
MontyDecoder().process_decoded(d)

@janosh
Copy link
Member

janosh commented Aug 31, 2023

Good question! Not obvious what's the best solution here. We could issue a silenceable runtime warning when users set non-JSONable properties.

@gpetretto
Copy link
Contributor Author

This is a good point. I actually did not think much about this kind of problems since in my view properties is somewhat equivalent to the site_properties, which suffer from the same limitation.
In general, adding non serializable properties could still be a valid use case, as long as the structure in not serialized. So, I would not strictly prevent a user from doing that. In any case one would get an error when trying to dump the structure to a file.

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

Successfully merging this pull request may close these issues.

Add structure_property attribute to Structure
3 participants