Towards #2468 (RDKit interoperability GSOC project)

[orbeckst]

Towards #2468 (RDKit interoperability GSOC project)
Replaces PR #2775

• Overview of work done in PR

  This PR adds the RDKitConverter class which converts MDAnalysis
  Universe/Atomgroup objects to RDKit rdchem.Mol objects.

• Limitations

  • Bonds and elements must be present (the former will be inferred if not
    present).
  • This converter mainly aims at supporting cases where explicit hydrogens
    are present.

• Extra implementation details

  • Bond order and formal charges are inferred via atomic valencies & the
    number of unpaired electrons (see _infer_bo_and_charges).
  • In part due to the influence of atom read order on inferring, bond
    conjugation and functional groups are standardized using SMARTS
    reactions (see _standardize_patterns).

• Other changes

  Also includes some PEP8 changes and some cleaning up of the tests for the
  RDKITReader.

• References
  For more information on the development process for this PR, see:

  1. https://cbouy.github.io/blog/2020/07/01/rdkit-converter
  2. https://cbouy.github.io/blog/2020/07/22/rdkit-converter-part2

[pep8speaks]

Hello @orbeckst! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file package/MDAnalysis/coordinates/RDKit.py:

Line 27:80: E501 line too long (105 > 79 characters)
Line 276:80: E501 line too long (103 > 79 characters)
Line 309:45: E128 continuation line under-indented for visual indent
Line 606:80: E501 line too long (99 > 79 characters)
Line 608:80: E501 line too long (99 > 79 characters)
Line 610:80: E501 line too long (99 > 79 characters)
Line 612:80: E501 line too long (99 > 79 characters)
Line 614:80: E501 line too long (99 > 79 characters)
Line 616:80: E501 line too long (99 > 79 characters)
Line 618:80: E501 line too long (99 > 79 characters)
Line 620:80: E501 line too long (99 > 79 characters)
Line 622:80: E501 line too long (99 > 79 characters)
Line 624:80: E501 line too long (99 > 79 characters)
Line 759:80: E501 line too long (82 > 79 characters)
Line 762:80: E501 line too long (88 > 79 characters)
Line 767:80: E501 line too long (86 > 79 characters)

• In the file testsuite/MDAnalysisTests/coordinates/test_rdkit.py:

Line 417:72: W291 trailing whitespace
Line 419:60: W291 trailing whitespace

[orbeckst]

This PR is the history cleaned-up version of PR #2775 (as a single commit).

[orbeckst]

@IAlibay can you please look after this PR? Thanks!

[IAlibay]

@IAlibay can you please look after this PR? Thanks!

Will do, thanks so much for fixing this @orbeckst !

Seems like everything is there, I'll have a detailed look in the morning.

[cbouy]

I don't know if it's necessary or not to mention this PR in the changelog?
Other than that, everything lgtm 👍

[orbeckst]

No, the other one is the important one. Thanks for reviewing!

…

On Aug 21, 2020, at 6:44 PM, Cédric Bouysset ***@***.***> wrote: I don't know if it's necessary or not to mention this PR in the changelog?

[codecov]

Codecov Report

Merging #2916 into develop will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2916      +/-   ##
===========================================
+ Coverage    92.89%   92.97%   +0.07%     
===========================================
  Files          187      187              
  Lines        24548    24787     +239     
  Branches      3182     3237      +55     
===========================================
+ Hits         22805    23046     +241     
+ Misses        1697     1693       -4     
- Partials        46       48       +2     

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/RDKit.py	97.31% <100.00%> (-2.69%)	⬇️
package/MDAnalysis/due.py	75.00% <0.00%> (+25.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f0381e...9ebc1b2. Read the comment docs.

[orbeckst]

Yay! 😎🤩🚀 Irfan, please merge at your leisure!

…

Am 22.08.2020 um 09:34 schrieb Irfan Alibay ***@***.***>:  @IAlibay approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[IAlibay]

@orbeckst so I don't mess up (again), I assume it's fine to just do a standard merge here? Considering the rebase + squash-merge (sort of), I'm not super clear on what's the best way to avoid losing @cbouy as the main author.

[orbeckst]

Standard merge should work. This won’t loose him as the author.

…

Am 22.08.2020 um 13:46 schrieb Irfan Alibay ***@***.***>:  @orbeckst so I don't mess up (again), I assume it's fine to just do a standard merge here? Considering the rebase + squash-merge (sort of), I'm not super clear on what's the best way to avoid losing @cbouy as the main author. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.