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 support for serializing ase Atoms objects #582

Closed
wants to merge 4 commits into from

Conversation

utf
Copy link
Contributor

@utf utf commented Oct 20, 2023

This PR adds support for serializing ase Atoms objects.

We've found this might be a useful feature to have in atomate2. But I think it could be useful more broadly. E.g., Atoms objects are used in the MD interface in matgl, in chgnet, and quacc (developed by @Andrew-S-Rosen). Although I understand if this sort of feature is outside the scope of monty.

@utf utf changed the title Add support for serializing atoms Add support for serializing ase Atoms objects Oct 20, 2023
@shyuep
Copy link
Contributor

shyuep commented Oct 20, 2023

I agree this is useful. But shouldn't this be implemented in ASE itself? Eg pymatgen implements Structure serialization. Monty is not exactly a materials science code. It is strange to have ASE serialization here.

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (3d6ba72) 75.56% compared to head (52a516f) 75.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
+ Coverage   75.56%   75.59%   +0.03%     
==========================================
  Files          27       27              
  Lines        1465     1475      +10     
  Branches      309      312       +3     
==========================================
+ Hits         1107     1115       +8     
- Misses        302      304       +2     
  Partials       56       56              
Files Coverage Δ
monty/json.py 85.13% <85.71%> (-0.16%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@utf
Copy link
Contributor Author

utf commented Oct 20, 2023

I agree that this feels quite specific for monty. I'll raise an issue on the ase repo and see if they would be happy with me adding monty support directly.

@Andrew-S-Rosen
Copy link
Contributor

Andrew-S-Rosen commented Oct 20, 2023

ASE does have its own .todict() and .fromdict() methods. That said, it doesn't currently work if there are constraints if I recall correctly. So, I usually have something like the following in my code that uses the ASE JSON encoder/decoder instead, although I believe the encoded object is a str type, which isn't necessarily ideal.

from ase.io.jsonio import decode, encode

def atoms_as_dict(s: Atoms) -> dict:
    return {"@module": "ase.atoms", "@class": "Atoms", "atoms_json": encode(s)}

def atoms_from_dict(d: dict) -> Atoms:
    return decode(d["atoms_json"])

Atoms.as_dict = atoms_as_dict
Atoms.from_dict = atoms_from_dict

Not sure of the pros/cons compared to your approach @utf. I'd need to investigate. But I just wanted to share what I already know about the topic.

@Andrew-S-Rosen
Copy link
Contributor

Andrew-S-Rosen commented Oct 20, 2023

I agree that this feels quite specific for monty. I'll raise an issue on the ase repo and see if they would be happy with me adding monty support directly.

I'm a maintainer of ASE now, so feel free to float something by me. I might be able to help get it through. My gitlab username is andrewrosen.

@shyuep
Copy link
Contributor

shyuep commented Oct 20, 2023

I would be fine adding support for todict and fromdict as alternatives to as_dict and from_dict. Though it would be better if ASE simply adds these aliases.

@shyuep
Copy link
Contributor

shyuep commented Nov 6, 2023

Closing this for now.

@shyuep shyuep closed this Nov 6, 2023
@Andrew-S-Rosen
Copy link
Contributor

Andrew-S-Rosen commented Feb 9, 2024

I know this is quite old, but I wanted to chime in to say that there is basically 0% chance such methods will be able to be added to ASE. It either has to happen in monty or monkeypatched in each code that wants to make Atoms an MSONable.

I would be fine adding support for todict and fromdict as alternatives to as_dict and from_dict.

Did we want to revisit this proposition? It is not necessarily ideal since monty is a general-purpose package, but it is indeed practical.

I will note that the method proposed by @utf is not ideal. The below will crash because FixAtoms is not JSON serializable.

from ase.build import bulk
from ase.constraints import FixAtoms
from monty.json import MontyEncoder

atoms = bulk("Cu")
c = FixAtoms([0])
atoms.set_constraint(c)
{"@module": "ase", "@class": "Atoms", "data": MontyEncoder().encode(atoms.todict())}

My approach using ASE's JSON encoder/decoder will always work, but it has the downside that the Atoms object data is stored as a JSON string, which is not readily parseable via a database query. You'd have to rehydrate it with an Atoms.from_dict() call before use.

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.

3 participants