-
Notifications
You must be signed in to change notification settings - Fork 1
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 function to calculate soap descriptors #74
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.
Been meaning to add DScribe myself, so I like this. docstring wouldn't hurt though.
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.
Docstring is also critical for a new tool, but for a tool like this it could be as simple as "Creates a dscribe.SOAP
object and creates descriptors from it, as documented below (if dscribe
is available:" and then (conditionally on dscribe
being available) append the the docs from SOAP
and SOAP.create
to the function's __doc__
attribute. Actually, I guess the structure
arg should still be documented, but the rest line up perfectly with the wrapped library so there's no need to re-write things here.
structuretoolkit/analyse/dscribe.py
Outdated
@@ -0,0 +1,44 @@ | |||
def calculate_soap_descriptor_per_atom( |
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.
Nit: I'd just go with stk.analyse.soap_descriptor_per_atom
as I'm not convinced calculate_
really adds any information
return periodic_soap.create( | ||
system=structure, | ||
centers=centers, | ||
n_jobs=n_jobs, | ||
only_physical_cores=only_physical_cores, | ||
verbose=verbose, | ||
) |
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 prefer
structure_soap_array = periodic_soap.create(...)
return structure_soap_array
More explicit and clearer to someone who is trying to figure out what the function does.
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.
That would actually integrate better with the workflow stuff too. There, we scrape the output channel label from whatever's after the return value -- giving it a nice variable like that keeps things extra tidy.
Check if it is possible to build |
No description provided.