Fixes a bug in AddHs() involving sp2 centers with degree 1 #3383
Conversation
|
The Windows CI failure is not a real one - failed download. The Windows DLL build passed. |
Code/GraphMol/molopstest.cpp
Outdated
| @@ -1976,6 +1976,100 @@ void testAddHsCoords() { | |||
| delete m; | |||
| delete m2; | |||
|
|
|||
| // make sure Hs are on the xy plane if conformer is 2D | |||
There was a problem hiding this comment.
This is ok as-is, but I've started (mostly) adding new test cases so that more "modern" style (like using _smiles) can be used.
Code/GraphMol/molopstest.cpp
Outdated
| auto nh2NbrRange = boost::make_iterator_range( | ||
| m2->getAtomNeighbors(m2->getAtomWithIdx(nh2Idx))); | ||
| std::vector<ROMol::vertex_descriptor> nh2His(2); | ||
| auto it = std::copy_if(nh2NbrRange.begin(), nh2NbrRange.end(), nh2His.begin(), |
There was a problem hiding this comment.
In the line above you've restricted the size of nh2His to be 2, but then you use this general approach to find the H neighbors. That feels inconsistent.
Since you know the order in which the Hs are added (and we don't say anywhere that you shouldn't rely upon this), I think it makes things simpler and considerably more readable to just specifically use the atom IDs of the Hs in the testing code
|
@greglandrum Done. Yeah, I went a bit over the top with the |
You can actually use an |
* Fixes a bug in AddHs() involving sp2 centers with degree 1 * Changes in response to review * forgot to run clang-format
Hs are not put in the same plane as the
sp2system the atom belongs to. This affects ethylene (2 heavies only) and conjugated systems of any size.This gist demonstrates the bug:
https://gist.github.com/ptosco/620edc8a3fd411928499204003eefb59
This gist shows the results obtained after merging this PR:
https://gist.github.com/ptosco/93f85d18794ed46da4122fa35bdb7b6e