New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add explicit hydrogens if needed in from_openeye #364
Conversation
Codecov Report
@@ Coverage Diff @@
## master #364 +/- ##
==========================================
+ Coverage 76.54% 76.56% +0.01%
==========================================
Files 19 19
Lines 5585 5594 +9
==========================================
+ Hits 4275 4283 +8
- Misses 1310 1311 +1
Continue to review full report at Codecov.
|
|
||
# Add explicit hydrogens if they're implicit | ||
if oechem.OEHasImplicitHydrogens(oemol): | ||
oechem.OEAddExplicitHydrogens(oemol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure how we should do this, actually. Does this change make from_smiles(..., hydrogens_are_explicit=True)
behave unexpectedly? I think that function calls from_openeye()
. Maybe from_openeye()
should take an argument to control this behavior as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. Great thinking, I had totally missed the possible consequences there.
So, the case where this would be a problem is if the user provided a SMILES with a specific explicit protonation state, and the toolkit misinterpreted it as having implicit hydrogens (even though the user set hydrogens_are_explicit=True
).
To prevent this from happening, I've updated both OpenEyeToolkitWrapper
and RDKitToolkitWrapper
's from_smiles
functions to assert that, if hydrogens_are_explicit=True
, then there are zero implicit hydrogens on the molecule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you! I've added only a few minor comments, but feel free to merge when you think it's ready.
Summary
Status