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

Adds support for an MSONAtoms class that's an MSONable form of an ASE Atoms object #3619

Merged
merged 7 commits into from
Feb 14, 2024

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Feb 13, 2024

Summary

Here, I introduce the MSONAtoms class, which is simply an MSONable version of the ASE Atoms object, introducing an .as_dict() and from_dict() method that uses ASE's JSON encode/decoder to do the serialization. By default, the object returned from ASEAtomsAdaptor.get_atoms() is now an MSONableAtoms object. The original Atoms class can be returned via the msonable=False keyword argument, although this should rarely be needed in practice since the functionality of MSONAtoms is otherwise identical to Atoms. This is a follow-up to materialsvirtuallab/monty#619.

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

@Andrew-S-Rosen Andrew-S-Rosen marked this pull request as draft February 13, 2024 06:26
@Andrew-S-Rosen Andrew-S-Rosen changed the title Make a (de)serializable PMGAtoms class Make a PMGAtoms class that is MSONable Feb 13, 2024
@Andrew-S-Rosen Andrew-S-Rosen changed the title Make a PMGAtoms class that is MSONable Make a PMGAtoms class that is an Atoms object that is MSONable Feb 13, 2024
@Andrew-S-Rosen Andrew-S-Rosen changed the title Make a PMGAtoms class that is an Atoms object that is MSONable Adds support for a PMGAtoms class that is simply an MSONable form of an ASE Atoms object Feb 13, 2024
@Andrew-S-Rosen Andrew-S-Rosen changed the title Adds support for a PMGAtoms class that is simply an MSONable form of an ASE Atoms object Adds support for a PMGAtoms class that's an MSONable form of an ASE Atoms object Feb 13, 2024
pymatgen/io/ase.py Outdated Show resolved Hide resolved
Signed-off-by: Andrew S. Rosen <[email protected]>
Signed-off-by: Andrew S. Rosen <[email protected]>
@Andrew-S-Rosen Andrew-S-Rosen changed the title Adds support for a PMGAtoms class that's an MSONable form of an ASE Atoms object Adds support for an MSONableAtoms class that's an MSONable form of an ASE Atoms object Feb 13, 2024
@Andrew-S-Rosen Andrew-S-Rosen marked this pull request as ready for review February 13, 2024 16:24
@shyuep
Copy link
Member

shyuep commented Feb 13, 2024

MSONAbleAtoms is a mouthful.How about just call it AtomsM?

@Andrew-S-Rosen
Copy link
Member Author

MSONAbleAtoms is a mouthful.How about just call it AtomsM?

My personal opinion is that descriptive is better than concise. It's not immediately clear what an AtomsM means at a glance. Having it be unambiguous is particularly helpful with type hinting, among other things.

That said, I agree it's somewhat long. I'm open to other alternatives.

@JaGeo
Copy link
Member

JaGeo commented Feb 13, 2024

I would also vote for MSONAbleAtoms or something descriptive.

@shyuep
Copy link
Member

shyuep commented Feb 13, 2024

Then MSONAtoms?

@Andrew-S-Rosen
Copy link
Member Author

Works for me. I'll make the change when I'm at my laptop.

Copy link
Member

@mkhorton mkhorton left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I do have a question, perhaps @Andrew-S-Rosen knows, is lossless conversion to/from Structure and Atoms possible? Or are some attributes potentially lost?

@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Feb 14, 2024

@mkhorton Good question. I have spent a fair bit of time on this module to ensure that it is lossless, and there are back-and-forth tests for this as well. If you spot an edge case, please let me know and I will patch it!

@Andrew-S-Rosen Andrew-S-Rosen changed the title Adds support for an MSONableAtoms class that's an MSONable form of an ASE Atoms object Adds support for an MSONAtoms class that's an MSONable form of an ASE Atoms object Feb 14, 2024
Signed-off-by: Andrew S. Rosen <[email protected]>
Signed-off-by: Andrew S. Rosen <[email protected]>
@janosh
Copy link
Member

janosh commented Feb 14, 2024

Or are some attributes potentially lost?

@mkhorton there's no remaining known loss of information during Atoms<->Structure round trips. many edge cases were reported in the past which have all been addressed, most recently in

#3359
#3338
#3270
#3151

unknown remaining edge cases are possible but likely rare

@shyuep shyuep merged commit ed965e9 into materialsproject:master Feb 14, 2024
22 checks passed
@shyuep
Copy link
Member

shyuep commented Feb 14, 2024

Thanks @Andrew-S-Rosen . This is great!

@shyuep
Copy link
Member

shyuep commented Feb 14, 2024

I just pushed a change that ensures MSONAtoms is returned by from_dict. This is supposed to be the case. Otherwise it loses functionality with deserializaation.

@janosh janosh added serialization Data/object serialization enhancement A new feature or improvement to an existing one ase Atomic simulation environment labels Feb 21, 2024
@@ -38,18 +40,41 @@
__date__ = "Mar 8, 2012"


class MSONAtoms(Atoms, MSONable):
Copy link
Contributor

Choose a reason for hiding this comment

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

Something here breaks imports in cases where the user doesn't have ASE installed. This seems to be breaking the latest mp_api as a result:

from mp_api.client.mprester import MPRester
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/mevans/repos/re2fractive/re2fractive/experiments/2_featurizing_refractive_index_data/.venv-mp-api/lib/python3.11/site-packages/mp_api/client/__init__.py", line 8, in <module>
    from .mprester import MPRester
  File "/home/mevans/repos/re2fractive/re2fractive/experiments/2_featurizing_refractive_index_data/.venv-mp-api/lib/python3.11/site-packages/mp_api/client/mprester.py", line 10, in <module>
    from emmet.core.electronic_structure import BSPathType
  File "/home/mevans/repos/re2fractive/re2fractive/experiments/2_featurizing_refractive_index_data/.venv-mp-api/lib/python3.11/site-packages/emmet/core/electronic_structure.py", line 11, in <module>
    from pymatgen.analysis.magnetism.analyzer import (
  File "/home/mevans/repos/re2fractive/re2fractive/experiments/2_featurizing_refractive_index_data/.venv-mp-api/lib/python3.11/site-packages/pymatgen/analysis/magnetism/__init__.py", line 5, in <module>
    from pymatgen.analysis.magnetism.analyzer import (
  File "/home/mevans/repos/re2fractive/re2fractive/experiments/2_featurizing_refractive_index_data/.venv-mp-api/lib/python3.11/site-packages/pymatgen/analysis/magnetism/analyzer.py", line 24, in <module>
    from pymatgen.transformations.advanced_transformations import MagOrderingTransformation, MagOrderParameterConstraint
  File "/home/mevans/repos/re2fractive/re2fractive/experiments/2_featurizing_refractive_index_data/.venv-mp-api/lib/python3.11/site-packages/pymatgen/transformations/advanced_transformations.py", line 33, in <module>
    from pymatgen.io.ase import AseAtomsAdaptor
  File "/home/mevans/repos/re2fractive/re2fractive/experiments/2_featurizing_refractive_index_data/.venv-mp-api/lib/python3.11/site-packages/pymatgen/io/ase.py", line 44, in <module>
    class MSONAtoms(Atoms, MSONable):
                    ^^^^^
NameError: name 'Atoms' is not defined

Will raise an issue

Copy link
Contributor

Choose a reason for hiding this comment

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

see #3644

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 needs testing PRs that are not ready to merge due to lacking tests serialization Data/object serialization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants