-
Notifications
You must be signed in to change notification settings - Fork 93
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
Throw error for "explicit" hydrogens that aren't in molecule graph #1983
Conversation
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.
This is fantastic - I think you nailed this. Some minor changes requested that I think are important, but I think the core of this is really solid. Nice work.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Thanks a million @jaclark5 - This is super thorough work and a great quality PR. Approved pending merging the changes proposed in this review and fixing the tests that we expect will break. Once the tests are green, you're clear to merge!
Co-authored-by: Jeff Wagner <[email protected]>
Co-authored-by: Jeff Wagner <[email protected]>
for more information, see https://pre-commit.ci
This PR flags with an error implicit hydrogens missed by RDKit, e.g., [NH+], as referenced in:
Issues resolved:
from_smiles
allows implicit H smiles when explicit H is True #936Resolves QCA-Dataset Submission Issue 327
This PR might partially be related to partially address QCS-Dataset-Submission Issue 2
This PR seeks to throw an error for unacceptable CMILES strings, why these strings are created is a topic of OpenFF-Toolkit Issue 1697, QCSubmit Issue 166 and Issue 216, and OpenFF-Sage Issue 2. Possible causes could be related to OpenFF-Toolkit Issue 900.
The solution contributed in this PR throw appropriate errors for molecules in the QCA-Dataset Submissions which were erroneously submitted: PR 207, PR 228 as outlined in OpenFF-Toolkit Issue #1696 (resolved in this PR)
Tag issue being addressed
Add tests
Update docstrings/documentation, if applicable
Lint codebase
Update changelog