Closed shuail closed 7 years ago
@shuail - we discussed doc strings a bit. Did you have any updates to those before I begin reviewing?
Also, can you explain what "erc score" is, in your description above?
@davidlmobley Oops, that's my fault, it should be ecr score which is corresponding to the ECR (EleCtrostatic Rule) in the original code. I will push a change to fix that and add some documentations in the README file to explain how to use the radial options. All should be done by today.
@davidlmobley fix some bugs and add more documentations in the README file to explain the radial graphing.
Looks like Travis tests are failing for something which is not your fault, @shuail -- I think @nividic had to build a graphviz for travis to use and it looks like it may have gone out of date. Is this something you want to look into or should I see if I can sort it out?
@davidlmobley Thanks for the checking and comments, I appreciate for that! I will try to cleanup the code based on comments here today or tomorrow and add a example case for the new graphing option. The next step would be add more unit tests in the test code. I am ok with either 1, merging right after the clean up or 2, after the new unit test being added. For the travis issue, I am not sure how to fix that instantly. So it'll be great if you have some ideas to easily bypass that otherwise, I will start to look into some details about travis. Thx!
Regarding Travis -- it's not an issue with travis specifically. Basically, Travis is just trying to install all the prerequisites itself (usually via conda) and then run our specified tests. It's having trouble installing one of the dependencies, so it fails. Presumably anyone else installing in the same, straightforward way that Travis is doing (see https://github.com/MobleyLab/Lomap/blob/master/.travis.yml and https://github.com/MobleyLab/Lomap/blob/master/devtools/conda-recipe/meta.yaml) will run into the same error.
@davidlmobley yes, I get some errors after running "conda build devtools/conda-recipe" under https://github.com/MobleyLab/Lomap/blob/master/.travis.yml The error is below:
ERROR:conda.core.link:An error occurred while installing package 'nividic::graphviz-2.38.0-1'. PaddingError: Placeholder of length '80' too short in package /home/shuai/miniconda/conda-bld/lomap_1501794001217/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/lib/graphviz/libgvplugin_pango.so.6.0.0.
@davidlmobley I just commit the code according to my comments and add an example code/folder to show the radial graphs. I will then work on the unit test and prepare an rdkit kekulized failure example.
@davidlmobley Some unit tests has been added to cover the radial graph option. Basically, I just follow the original pattern in the test_lomap.py code to ensure that the code will generate identical graphs between 1, the saved graph (generate using the current code) and 2, the graph regenerated by the unit test code. Also I add an problematic case in the issue tracker #26 to show the issue where the rdkit cannot kekulize the molecule. So I think the code is good for you to review. Thanks!
@davidlmobley Yes, the code pass the unit test locally and ready to merge I think. Yes, I guess the travis issue is related to the version of graphviz in the Gaetano's channel. If that is not an easy fix, we could consider to rebuild a conda channel? Let me know if you want me to do that, I could chip in and check out more details for that.
@shuail @davidlmobley I have update my graphviz conda repository. Travis is not complaining anymore. Unfortunately I tried to install lomap locally and I had some issues. To make it to work I had follow this link https://github.com/awslabs/aws-shell/issues/161. The problem is related to OSX only. Please let me know if you have the same problem or not. On linux it seems to work.
I'm closing and re-opening this PR to re-trigger the tests. @shuail - if the tests pass you should be good to merge.
@nividic Thanks for updating the graphviz! @davidlmobley all checks have passed this time!
@shuail - tests passed; merging. Thanks!
Based on our needs, I modify the code to output more information (score, filename, connectivity etc) and add more similarity scores (tanimoto score), I also try to change some threshold based on the experiences of running the code towards our test set. For graphing part, I add couple radial options to create radial shape graphs. Below are the details:
1, add erc score (charge score) to DBMolecules database (dbmol.py) 2, change the total charge comparison threshold from 1e-3 to 1e-2 (dbmol.py) 3, add a function to calculate the tanimoto score (mcs.py) 4, add radial graphing option (in both dbmol.py and graphgen.py): Complete radial option will pick the radial center (lead compound) automatically by the structure similarity: the ligand shared the most structural similarity with others will be picked as lead compounds Biased radial option will take the custom specified compound as the lead compound. 5, output the pickle file containing DBMolecules database with graph info. (dbmol.py) 6, change the max_mol_size from 11 to 50 angstrom in the graphing to accommodate larger molecule. (graphgen.py) 7, improve the drawing function to layout the 2D structure. (graphgen.py) 8, add the layout_info function in the graphgen.py to output ligand index, filename, multiple scores and connectivities (graphgen.py)