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 ASE Atoms support for de/encoding #619

Closed
wants to merge 9 commits into from

Conversation

Andrew-S-Rosen
Copy link
Contributor

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

Summary

Note: will fix test shortly.

This is a follow-up to #582 by @utf with the goal of adding support for Atoms encoding/decoding via the MSONable spec. For pros/cons of this, refer to the original discussion of #582.

@utf: If you have a better mechanism to serialize the Atoms that results in fully reversible (de)serialization without relying on storing the data as JSON strings, let me know. The mechanism you had originally proposed in #582 does not work when Atoms has non-JSON serializable components, like when a FixAtoms constraint is applied.

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)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

@shyuep
Copy link
Contributor

shyuep commented Feb 9, 2024

I don’t mean to be difficult, but I really think this is not suitable for Monty. It is far too materials science specific while Monty has nothing related to matsci at all. In the end, the guiding principle cannot be that “ASE developers are unwilling to do it, so let’s shove it to some other codebase where it is definitely unsuitable”. At most, I will tolerate some version of it in pymatgen. E.g., we create a subclass of Atoms called PymatgensBetterAtoms that implements as_dict and from_dict and supports MSONAble. This can be used in place of all places that use Atoms. It also has the advantage that we can implement whatever additional functionality we want without having to deal with the whims of other devs.

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

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

Comparison is base (a4573e3) 0.00% compared to head (c7cae2a) 78.95%.

Files Patch % Lines
monty/json.py 20.00% 6 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #619       +/-   ##
===========================================
+ Coverage        0   78.95%   +78.95%     
===========================================
  Files           0       27       +27     
  Lines           0     1416     +1416     
  Branches        0      302      +302     
===========================================
+ Hits            0     1118     +1118     
- Misses          0      242      +242     
- Partials        0       56       +56     

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

@Andrew-S-Rosen Andrew-S-Rosen marked this pull request as draft February 9, 2024 19:08
@Andrew-S-Rosen
Copy link
Contributor Author

I don’t mean to be difficult, but I really think this is not suitable for Monty. It is far too materials science specific while Monty has nothing related to matsci at all. In the end, the guiding principle cannot be that “ASE developers are unwilling to do it, so let’s shove it to some other codebase where it is definitely unsuitable”. At most, I will tolerate some version of it in pymatgen. E.g., we create a subclass of Atoms called PymatgensBetterAtoms that implements as_dict and from_dict and supports MSONAble. This can be used in place of all places that use Atoms. It also has the advantage that we can implement whatever additional functionality we want without having to deal with the whims of other devs.

In short: I, in fact, agree. I will respond more over email to your other inquiries.

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.

2 participants