Refactor ring methods in Molecule class

[mattwthompson]

Fixes #823

This PR tries to implement solutions to #823. The major change is that several properties Molecule.rings, .n_rings, Atom.is_in_ring, Bond.is_in_ring, have been converted to methods, which breaks the existing API. The ways that membership in rings was being checked wasn't necessarily the best, since RDKit exposes a more direct way to do this (I assume OEChem does as well).

The main problem with the earlier implementation is that storing rings in Molecule._rings loses the provenance of which toolkit determined the number of rings (RDKit, OpenEye, and other toolkits are not guaranteed to agree). So these methods now generate rings on the fly as needed instead of storing them as private attributes and exposing getters. This should work around edge cases resulting from toolkit differences (unclear how we can test these without OpenEye functionality added). There an obvious downside of this change is wasteful re-computing the rings. I don't know if this is an important performance hit or not.

• Refactor Bond.is_in_ring to use direct cheminformatics toolkit calls
• Deprecate Molecule.rings and Molecule.n_rings
• Tag issue being addressed
• Add tests
• Update docstrings/documentation, if applicable
• Lint codebase
• Update changelog

[codecov]

Codecov Report

Merging #855 (137139c) into master (f9d8ee7) will decrease coverage by 0.33%.
The diff coverage is 95.00%.

[mattwthompson]

The toolkit wrappers are disagreeing on the number of rings in naphthalene:

In [35]: molecule = Molecule.from_smiles("c1ccc2ccccc2c1")

In [36]: oechem.OEDetermineRingSystems(molecule.to_openeye())
Out[36]: (1, [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0])

In [37]: rdmol = molecule.to_rdkit(); rdmol.GetRingInfo().AtomRings()
Out[37]: ((0, 1, 2, 3, 8, 9), (4, 5, 6, 7, 8, 3))

In [38]: molecule.get_n_rings(toolkit_registry=OpenEyeToolkitWrapper())
Out[38]: 1

In [39]: molecule.get_n_rings(toolkit_registry=RDKitToolkitWrapper())
Out[39]: 2

but the other methods are behaving well in some simple tests.

[mattwthompson]

Looks like a test is failing due to a toolkit disagreement in how many rings a molecule has. I'll look into that tomorrow, probably just removing it since it's a deprecated API point - otherwise this is ready for review @j-wags

[j-wags]

Sorry to be nitpicky here, but I can't meaningfully review this if we're planning to fully remove Molecule.rings and n_rings before merging (like I think we've decided to do, though maybe we understood different things from the discussion). Most of the lines in the test diffs will be substantially changed and so the review would be huge and it'd be hard to keep track of everything under discussion.

I think we should immediately remove those API points with no deprecation warning. It's bad, but we don't have a release planned where anyone would see a deprecation warning anyway, and 0.11.0 will hopefully be our only major API break this year, so this is probably the most realistic way forward.

[mattwthompson]

My read of the discussion was that we'd deprecate those points in this release with the intention of removing them later - probably since that's what we would typically do. I will update this to remove them now, though.

[j-wags]

This is starting to come together really well. I think we can simplify the toolkitwrapper method signatures and avoid a lot of weird edge cases in the process -- See suggested code changes.

[j-wags]

Thanks for all the iteration on this. I'm approving this PR, pending changes in the comment before this post.

[lgtm-com]

This pull request introduces 1 alert when merging c413f35 into a7a9f18 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com]

This pull request introduces 1 alert when merging 475abc7 into a7a9f18 - view on LGTM.com

new alerts:

• 1 for Unused import