search

MobleyLab / chemper

Repository for Chemical Perception Sampling Tools
MIT License
19 stars 10 forks source link

API change, major doc string update, and cleaning up READMEs #82

Closed bannanc closed 4 years ago

bannanc commented 5 years ago

In this PR I will do minor cleaning of the README (formatting, see PR #26 for more extensive clean-up). Then I'm trying to close some more minor things from the issue tracker. Here is what is done so far:

80 - Update badges to show which operating systems are being tested and format with logo to be less dense

78 - use oechem.OEChemIsLicensed() to check for OE_LICENSE file, not just that it is installed

Things I plan to address in this PR

codecov[bot] commented 5 years ago

Codecov Report

Merging #82 into master will decrease coverage by 2.26%. The diff coverage is 89.57%.

bannanc commented 5 years ago

I know I broke things on this most recent commit, I'm hoping that looking at the diff up here will help me understand how to fix it, but I can't figure out what I changed that would affect the actual code.

bannanc commented 5 years ago

This pull request introduces 1 alert and fixes 1 when merging 2e0b018a1239c3ff1afd51d281cd1d119a582d1a into 18ce149682af998683f5f9302af8f1d860fa69bf - view on LGTM.com

new alerts:

fixed alerts:

Comment posted by LGTM.com

bannanc commented 5 years ago

This pull request introduces 1 alert when merging 6bd24f19fcad5671f6bf41194cb474cb0ae9a5d7 into 18ce149682af998683f5f9302af8f1d860fa69bf - view on LGTM.com

new alerts:

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

bannanc commented 5 years ago

This pull request introduces 1 alert when merging abbb0f645c487fd8178ce17da54357f8ead89ab1 into 18ce149682af998683f5f9302af8f1d860fa69bf - view on LGTM.com

new alerts:

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

bannanc commented 5 years ago

This pull request introduces 1 alert when merging 331689f21fdd32f15d3aa738ba267db2013197d6 into 18ce149682af998683f5f9302af8f1d860fa69bf - view on LGTM.com

new alerts:

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

bannanc commented 5 years ago

This pull request introduces 1 alert when merging a90ff4d1861c1ba7b3304da20364165b2edd4339 into 18ce149682af998683f5f9302af8f1d860fa69bf - view on LGTM.com

new alerts:

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

bannanc commented 5 years ago

This pull request introduces 1 alert when merging 8a7bdd0700e7fcdfdb9cd844b77c14e32d6737b8 into 18ce149682af998683f5f9302af8f1d860fa69bf - view on LGTM.com

new alerts:

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

bannanc commented 5 years ago

This pull request introduces 1 alert when merging 029eb027b52a9064e95e815aacc7adba8e539e13 into 18ce149682af998683f5f9302af8f1d860fa69bf - view on LGTM.com

new alerts:

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

bannanc commented 5 years ago

This pull request introduces 1 alert when merging 39d72c820b2913d69defba1da8d1940fab5f6413 into 00d0112ecba538051e2fa5a3faef7e8b80d3e818 - view on LGTM.com

new alerts:

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

j-wags commented 4 years ago

Now I understand why we tried to move this into OFFTK! I think I've resolved the py37 errors by making the mol_toolkit.Mol class have the same signature as the cp_openeye and cp_rdkit mols. 4641e27 does this by providing stubs of the equivalent functions in the mol_toolkit.Mol class (and makes it explicitly inherit from MolAdapter so changes will track in the future).

However, I'm a bit nervous about the design pattern where we change self.__class__ inside the __init__ function itself (though I do understand why we do it, and wasn't able to think of a good alternative). I've added the following TODO (tl;dr -- We should see if we can make this use OFFMols):

    # TODO: This is a really interesting implementation. It basically says "I inherit from MolAdapter, and -- trust me
    #  -- whatever you get out of this will have all the MolAdapter functionality implemented". The class is basically
    #  just a switchboard to figure out which OTHER MolAdapter subclass is appropriate to the input. This design pattern
    #  makes me nervous because it would seem to break a contract with the user, where they call one class's __init__
    #  function, but receive an object of a different class.
    #  Alternatives to this would include
    #       * Having each MolAdapter subclass have a `from_object` method, similar to OFFTK. Then, the MolAdapter
    #         constructor could iterate over all subclasses (using something like an `all_subclasses` method) and
    #         try creating each class from the input, until one succeeds.
    #       * Just use the OFF Molecule class, which avoids this trouble by copying the data OUT of the OEMol or RDMol
    #         into a toolkit-independent format (OFFMol). Then, when manipulation operations occur, the OFFMol is
    #         converted back into the appropriate toolkit molecule, and the operation happens natively there. I think
    #         that smirks_search is the only function that would need to be ported over if that was done.
j-wags commented 4 years ago

That said, both implementations above are somewhat heavy lifting, and I'm not sure that a refactor is called for at this stage. For now, this works, and that's probably good enough until this gets another full-time maintainer.

bannanc commented 4 years ago

For the record, Jeff and I talked and he was going to help get this up to passing so we could merge the PR. I think of the two options above 2 makes more sense (use OFFTK) in the long run, but you have to have access to things like how many hydrogens and what is the connectivity of an atom which right now OFFTK doesn't have. There's no point in heavy lifting to change anything here until someone has done more testing to see if this SMARTS generation is what will ultimately end in the pipeline.

It looks like Travis is passing now, but Appveyor is failing because it can't get RDKit? I think I followed OFFTK to get Appveyor to work in the first place, but I'm not convinced that window support is necessary at this stage for this tool.

j-wags commented 4 years ago

Yeah, sorry, I meant to ask whether Windows support was important to you, @bannanc . It's not critical to me so I'm fine to skip it as well.

I'm happy to give this a quick code review if you think this is ready to go, though I probably won't finish until after the long Thanksgiving weekend. Also, I'm really unfamiliar with this code, so I think it's fine to merge without my input if you think it's ready.

bannanc commented 4 years ago

If the travis tests are passing with OE and RDK then I'm fine, but I don't want a failing badge on the README so I think this needs 3 things before I would want to merge:

bannanc commented 4 years ago

I'm fine with keeping the Appveyor integration so someone else can fiddle with it in the future, but I don't think its worth the hassle right now.

j-wags commented 4 years ago

Sounds good. Let me know if you want my input/help, otherwise I'll stay out of the way :-)

bannanc commented 4 years ago

@j-wags do you know why the codecov isn't finished? Is it trying to use the appveyor tests?

bannanc commented 4 years ago

Also, I triggered a rebuild on Appveyor to see if that's all it needed

bannanc commented 4 years ago

As a quick update, it looks like AppVeyor just needed to be rerun. I'll add the preprint to the README and then merge this branch and cut a release