-
Notifications
You must be signed in to change notification settings - Fork 6
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
Integrate with AtomsBase 0.3 #29
Conversation
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.
This looks good, thank you!
My one reservation is that I don't like the use of hard-coded lists of properties and the consequent fixed mapping of particular string names to units. This is OK for the truly mandatory ExtXYZ properties (basically just lattice, positions, chemical species) and by extension to the other 'standard' AtomsBase properties but I'm not sure how to deal with anything beyond this.
I suppose just leaving them unitless as you have done is the only thing we can do.
If you confirm
yeah this is tricky. My reasoning is that: If there is a preferred name that has a well-adjusted meaning, then it ought to have a well-defined unit as well. So you are bound to check this explicitly. In the long run (when the list gets longer) we could provide proper constants in |
@jameskermode I changed the units as discussed in the other PR. Let me know if you have other things to be done before merging this. |
Happy to merge now. Thanks for the contribution! |
Release 0.1.10 tagged and will show up in General registry shortly. |
Thanks! |
Closes #26, additionally: