-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Added Jmol colors as defaults for atoms. #682
Conversation
Could you update your black version, and run it with the newest one, as you can see the lint action fails, I noticed in ab45ef4 that black changed substantial things. |
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.
Just a small question, I haven't looked at how you are using it, but perhaps a defaultdict
would make this simpler?
@@ -66,15 +66,117 @@ def AtomNOrbitals(geometry, atoms=None): | |||
|
|||
|
|||
class AtomDefaultColors(AtomData): | |||
# Taken from Jmol (https://jmol.sourceforge.net/jscolors/) | |||
_atoms_colors = { |
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.
would it make sense to make this a defaultdict
?
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.
Why?
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.
It's used like this, right below the dictionary:
def function(self, geometry, atoms=None):
return np.array(
[
self._atoms_colors.get(atom.symbol, self._atoms_colors["else"])
for atom in geometry.sub(atoms).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.
Ok, then it would just be:
self._atoms_colors[atom.symbol] ...
and on .get
it will always be present.
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.
Hmm but a defaultdict
is meant to initialize objects when the key is not there. Isn't it a bit overkill for 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.
Not a problem then, it would make its use simpler since one shouldn't care if the key is there or not. I'll merge.
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.
I can convert it to something like self._atoms_colors.get( atom.symbol, self._fallback_color)
if it looks better.
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.
Not a problem then, it would make its use simpler since one shouldn't care if the key is there or not. I'll merge.
Ok, anyway this can be modified whenever we want :)
Ok, done. Again, pre-commit would be a good idea because it specifies the version :) |
Yes, lets do this. :) |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #682 +/- ##
==========================================
- Coverage 87.62% 86.14% -1.48%
==========================================
Files 363 364 +1
Lines 48870 43511 -5359
==========================================
- Hits 42820 37483 -5337
+ Misses 6050 6028 -22 ☔ View full report in Codecov by Sentry. |
I also ran black and modified some unrelated files 😅