-
Notifications
You must be signed in to change notification settings - Fork 874
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
Breaking: Update AseAtomsAdaptor
to handle Structure.properties
/Molecule.properties
#3270
Merged
+54
−32
Merged
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
2b95b83
better handling of properties
Andrew-S-Rosen f351960
update tests
Andrew-S-Rosen 6e98ccf
Merge branch 'master' into aseadapter
shyuep cb93163
Merge branch 'master' into aseadapter
Andrew-S-Rosen f46f7a4
patch
Andrew-S-Rosen 22ea3db
pre-commit auto-fixes
pre-commit-ci[bot] 8e1315a
patch
Andrew-S-Rosen 97ad081
fix
Andrew-S-Rosen e515e8f
fix
Andrew-S-Rosen 7def8bf
pre-commit auto-fixes
pre-commit-ci[bot] 8d73db2
jsanitize the atoms info
Andrew-S-Rosen f18c847
Merge remote-tracking branch 'origin/aseadapter' into aseadapter
Andrew-S-Rosen 750c211
add attr properties: dict to SiteCollection
janosh daec63a
breaking: compare self.properties == other.properties in IStructure._…
janosh 2c75998
fix TestMolecule.test_to_from_file_string()
janosh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
jsanitize the atoms info
commit 8d73db2f89f52ca184107beb64be94bfaf5bb27f
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janosh: Just wanted to point out one subtlety to you. Currently, we have the mapping:
Atoms.info <--> Structure.properties
, since they serve the same purpose between ASE and Pymtatgen, respectively. However, Ijsanitize
-d theatoms.info
before assigning it to.properties
to ensure theStructure
/Molecule
object can be (de)serialized since this is often an implicit assumption ofStructure
/Molecule
objects but is not necessarily one for ASEAtoms
objects. In practice, this means that there could be some scenarios where a user interconverts between ASEAtoms
and PymatgenStructure
/Molecule
with some loss of information; namely, if theatoms.info
is modified byjsanitize()
then it can't be restored in its original form. There might be a better solution than this, but I implemented it because oftentimes ASE will dump un-serializable class objects (namely, aSpacegroup
class) intoatoms.info
, such as when reading in a CIF with symmetry flags.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for highlighting! Seems like a good way to go about this for now. I'll keep this in mind if we run into any issues.