-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix issue #287 #290
Merged
Fix issue #287 #290
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mb2055
approved these changes
Feb 12, 2025
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.
All good 👍
lohedges
added a commit
that referenced
this pull request
Feb 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR closes #287 by adding a map option to force the fresh inference of stereochemistry when converting to RDKit format. Previously there was a bug which meant that the code always took this pathway, meaning that it didn't always preserve what was in the original SDF file. This new approach allows the user to take both approaches, then compare the result in RDKit, i.e. they can see if the information in the file agrees with what the inference suggests.
The PR also closes #289 where the
formal_charge
property of an atom was set incorrectly for certain positive charges. This is because the value in the SDF file isn't the actual charge, e.g:For example, a value of 3 in the file meant that an atom had +3 charge, not +1. This was ultimately the source of several BioSimSpace parameterisation issues, e.g. like the one posted here. While I had correctly fixed the conversion of the formal charge property back to the SDF field, I hadn't noticed the same bug was present when setting the property in the first place. This means that all parameterisation starting from SDF since BioSimSpace 2024.3.0 will have been incorrect if there was an atom with +1 charge (SDF value 3). As such, it might be worth testing some know systems to see if anything has changed.
The fix to the formal charge has meant that we can re-enable an XFAILED RDKIt SMARTS inference test that was disabled following the previous edits. Those meant that it no longer passed, but the formal charge was the underlying culprit.
devel
into this branch before issuing this pull request (e.g. by runninggit pull origin devel
): [y]