-
Notifications
You must be signed in to change notification settings - Fork 31
Ryan/dev work #30
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
base: master
Are you sure you want to change the base?
Ryan/dev work #30
Conversation
BingqingCheng
left a comment
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.
Make the default of --keepraw to False. And then we are ready to merge.
Good work!
| @@ -100,13 +100,14 @@ def main(fxyz, dictxyz, prefix, output, peratom, fsoap_param, soap_rcut, soap_g, | |||
| parser = argparse.ArgumentParser() | |||
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.
Cosmetics
| @@ -74,7 +74,8 @@ def plot_density_map(X, z, fig=None, ax=None, | |||
| """ | |||
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.
Cosmetics
|
|
||
| def normalizekernel(kernel): | ||
| # first normalize the kernel matrix | ||
| """ |
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 is the module I wrote the tests for. Just expanding the docstrings slightly.
| @@ -1,6 +1,6 @@ | |||
| #!/usr/bin/python3 | |||
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.
If the original kpca.py script is now deprecated shall we make this deprecated switch compatible with the photoswitch example or would you prefer a modification to the new kpca? (Wherever that might be, I haven't looked for it yet!)
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.
You can implement the Morgan fingerprints as a Global_Descriptor class in descriptors/global_descriptors.py
| @@ -50,7 +50,8 @@ def main(fmat, fxyz, ftags, fcolor, colorscol, prefix, output, peratom, keepraw, | |||
| if fxyz != 'none': | |||
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.
Cosmetics
| @@ -0,0 +1,72 @@ | |||
| """ | |||
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.
Very basic tests for the functions in the ml_kernel_operations.py module but useful as a template for further tests.
|
|
||
| from asaplib.kernel.ml_kernel_operations import kerneltodis, kerneltodis_linear, kerneltorho, normalizekernel | ||
|
|
||
|
|
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.
The @pytest.mark.parametrize decorator allows you to specify multiple test inputs to be run with pytest. In the case of the kernel function tests, we pass in different types of kernel as the test input and check that the output satisfies some basic properties that we'd expect to 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.
The passing of the tests, while not a formal correctness proof, ensures that the functions we write produce the correct output on the examples we specify. It's possible that a pathological input exists which will cause the functions to fail, but we try to think of all of these pathological inputs in advance, test them here and if there's a failure, modify the function.
BingqingCheng
left a comment
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.
Let's not keep the png files in the examples. As we have changed the design a lot, so the commands and the outputs are very different now.
| @@ -1,6 +1,6 @@ | |||
| #!/usr/bin/python3 | |||
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.
You can implement the Morgan fingerprints as a Global_Descriptor class in descriptors/global_descriptors.py
…t argument because it doesn't seem to be used anymore.
|
|
||
| import numpy as np | ||
| from rdkit.Chem import MolFromSmiles | ||
| from rdkit.Chem.AllChem import GetMorganFingerprintAsBitVect |
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.
Can we import the package inside the class?
| return {'acronym': self.acronym, 'descriptors': self.cm.create(frame, n_jobs=8)}, {} | ||
|
|
||
|
|
||
| class FingerprintDescriptor: |
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.
The logic of ASAPXYZ class is that the create() function for the descriptors takes a frame as argument.
So in this case,
create(self, frame):
compute_morgan(frame.info['smiles'])
Photoswitch example. Summary of changes that touch other parts of the codebase:
Extra flags added to
kpca.pyandumap_reducer.pyin order to allow the usage of the Morgan fingerprint representation. Default is not to use it.'metric' flag added to
gen_soap_kmat.pythat allows users to specify what form of kernel they want from kpca. Default set to linear.