-
Notifications
You must be signed in to change notification settings - Fork 15
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
Get chemical symbols #100
Get chemical symbols #100
Conversation
Pull Request Test Coverage Report for Build 617775079
💛 - Coveralls |
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.
lgtm
I like this choice. My rationale is this:
|
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.
Cool, cool cool cool.
Change suggestions in the review, but nothing barring merging IMO, just see what you like.
@property | ||
def chemical_symbols(self): | ||
""" | ||
Returns chemical symbols of the neighboring atoms. |
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.
codacy is going to whine that this could be on one line. I don't actually care strongly about this formatting choice, but I do like making codacy shut up 😉
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.
Oh ok I didn't know that.
@@ -339,6 +339,13 @@ def test_norm_order(self): | |||
with self.assertRaises(ValueError): | |||
neigh.norm_order = 3 | |||
|
|||
def test_chemical_symbols(self): |
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.
❤️ tests
I added
neigh.chemical_symbols
which gives the chemical symbols of the neighboring atoms.I was not sure if I should call it
neigh.get_chemical_symbols()
or make it a property. Since there's no argument to add, I decided to make it a property, but I'm open to suggestions.