Aromaticity and Gasteiger charges guessers #2926
Conversation
|
@richardjgowers @IAlibay @fiona-naughton Not sure who I'm supposed to ping now that GSoC ended 😅 |
There was a problem hiding this comment.
Overall lgtm, but needs tests and usage docs (there's some example use docstring at the top that could be good to add to, although we might want to migrate to user guide, @lilyminium thoughts?).
Couple of comments, there's a long term thought about how we want our guessers to look like in a multi-converter world. Overall we might need to completely overhaul the guessers, so I'm not convinced it needs to be addressed here.
| return np.array([atom.GetIsAromatic() for atom in atoms]) | ||
|
|
||
|
|
||
| def guess_gasteiger_charges(atomgroup): |
There was a problem hiding this comment.
I think for now it's fine, although long term we might want to consider how we want this to look API-wise. In a hypothetical world where we have converters for things like openbabel, we might end up with multiple possible charge assignment methods. It'd be nice to have them all accessible from the same entry point.
|
Forgot to add; can't speak for everyone, but I don't mind if you keep pinging me for the RDKit things. I get the email notifications whether or not you do so anyways :) |
Codecov Report
@@ Coverage Diff @@
## develop #2926 +/- ##
========================================
Coverage 92.81% 92.81%
========================================
Files 170 170
Lines 22801 22811 +10
Branches 3239 3243 +4
========================================
+ Hits 21162 21172 +10
Misses 1591 1591
Partials 48 48
Continue to review full report at Codecov.
|
|
Slowly going back into the RDKIT stuff. @cbouy if you're still interested in progressing here, I think what's stalling the PR is the lack of tests. |
Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Irfan Alibay <[email protected]>
There was a problem hiding this comment.
This looks fine to me. @lilyminium can you have a glance at this?
|
Thanks @cbouy ! -- squash merging |
Towards MDAnalysis#2468 ## Work done in this PR * Add aromaticity and Gasteiger charges guessers which work via the RDKIT converter.
* added get_connections * modified tests for atoms.bonds/angles/dihedrals etc * modified parsers and things to use get_connections or bonds * updated CHANGELOG * pep8 * undo half of PR 3160 * add intra stuff * Update package/MDAnalysis/core/groups.py Co-authored-by: Jonathan Barnoud <[email protected]> * tighten up base class checking * update docstring * suppres warnings * Use absolute file paths in ITPParser (#3108) Fixes #3037 Co-authored-by: Lily Wang <[email protected]> * Adds aromaticity and Gasteiger charges guessers (#2926) Towards #2468 ## Work done in this PR * Add aromaticity and Gasteiger charges guessers which work via the RDKIT converter. * BLD: handle gcc on MacOS (#3234) Fixes #3109 ## Work done in this PR * gracefully handle the case where `gcc` toolchain in use on MacOS has been built from source using `clang` by `spack` (so it really is `gcc` in use, not `clang`) ## Notes * we could try to add regression testing, but a few problems: - `using_clang()` is inside `setup.py`, which probably can't be safely imported because it has unguarded statements/ code blocks that run right away - testing build issues is typically tricky with mocking, etc. (though in this case, probably just need to move `using_clang()` somewhere else and then test it against a variety of compiler metadata strings * Remove ParmEd Timestep writing "support" (#3240) Fixes #3031 * Adding py3.9 to gh actions CI matrix (#3245) * Fixes #2974 * Python 3.9 officially supported * Add Python 3.9 to testing matrix * Adds macOS CI entry, formalises 3.9 support * fix changelog * special metaclass * move function down * tidy code Co-authored-by: Jonathan Barnoud <[email protected]> Co-authored-by: Aditya Kamath <[email protected]> Co-authored-by: Cédric Bouysset <[email protected]> Co-authored-by: Tyler Reddy <[email protected]> Co-authored-by: Irfan Alibay <[email protected]>
Part of #2468
Changes made in this Pull Request:
TopologyAttrPR Checklist