search

MobleyLab / Lomap

Alchemical mutation scoring map
MIT License
37 stars 17 forks source link

Merge the changes in stx-shuai branch to the main LOMAP branch #28

Closed shuail closed 6 years ago

shuail commented 6 years ago

1, add fast graphing option 2, add an option to use fingerprint for similarity calculations 3, few improvement in result rendering, like add a output file to writeout the morphing pairs, increasing the maximum number of images showing in the graph, output a pdf file for better resolution 4, update the unit test and README file

davidlmobley commented 6 years ago

Totally forgot about this PR. Is it ready to be merged? (But tests are failing; is that due to this PR or a separate issue?)

shuail commented 6 years ago

Oh, I just commit some codes into my repo which is irrelevant of the previous PR and since the PR is still here, the PR get updated. I totally forgot about the PR also, I think the original PR and the commit both failed the test showing by travis, will take a deep look of this and update the code if needed

shuail commented 6 years ago

@davidlmobley the tests was running for me to run locally and the travis here complains about the networkx and it looks like travis is using networkx version 2.0 and my local networkx is version 1.1. Do you have any ideas about how travis control the version of the site packages?

davidlmobley commented 6 years ago

@shuail - yes, Travis basically does a clean install of everything in a virtual machine as part of running the tests, so it is always using the latest version of each library unless we tell it to do something differently. For example, we could pin it to networkx 1.1, which would "fix" any networkx problems. However, that's probably not the right thing to do long-term.

Specifically, what this is telling us, then, is that the API changes NetworkX made going from 1.1 to 2.0 break LOMAP. And sooner or later we will have to adjust to compensate, as eventually we'll NEED networkx 2.0.

What we could do, if you like, is temporarily pin this to networkx 1.1 to verify that everything ELSE in this PR works properly, then get this merged and then work on the networkx changes in a separate PR. Would that appeal?

shuail commented 6 years ago

@davidlmobley that makes sense to me, that reminds me a hard time to install pyqt4 which required by lomap while the latest pyqt is version 5. So yes, could you pin lomap to networkx 1.1 and see if travis agree to merge. And I am interested in testing lomap using network 2.0, pyqt5 etc.

davidlmobley commented 6 years ago

To pin to a specific version, @shuail , you edit the .travis.yml and .meta.yml files (as applicable) to specify the specific version needed, e.g. see this meta.yml which requires at least 0.7.3 of openmoltools: https://github.com/openforcefield/openforcefield/blob/master/devtools/conda-recipe/meta.yaml#L24

davidlmobley commented 6 years ago

Can you take a stab at that? I'm out sick.

shuail commented 6 years ago

@davidlmobley change the meta.yaml to pin networkx version 1.11, will see how it works. (sorry in the previous message, I mentioned networkx 1.1 while I am actually using networkx 1.11)

shuail commented 6 years ago

@davidlmobley the testing failed and it seems the conda reinstall networkx 2.0 in line 856 https://travis-ci.org/MobleyLab/Lomap/jobs/293161761#L1552 , I think we may need to force pin the conda package to use networkx 1.11 as well. And it seems that networkx 2.0 released on Sep 2017 that probably explain why we didn't have this similar error before

davidlmobley commented 6 years ago

Yes, you'd want to change the networkx version in both places.

shuail commented 6 years ago

@davidlmobley all tests passed this time!

davidlmobley commented 6 years ago

OK, @shuail ; let's get this merged because it's overdue and you've been using it for a while. But can you create an issue to fix the networkx interface to be compatible with the new API? And in the future we should work to have more of a "one change per PR" model and apply some of the things you've been learning working on alchemlyb.

Also, perhaps we should think about getting code coverage testing in place here too?

shuail commented 6 years ago

@davidlmobley Sounds good to me, I will create a issue to explicitly address the networkx version. Yes, one change per PR would be ideal. There is no existing issue related to this pull request. I could create issues and link them to this pull request for a record, how about that? Yes, I like the code coverage idea.