Skip to content

Fix issue #392 #393

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

Merged
merged 4 commits into from
Feb 19, 2025
Merged

Fix issue #392 #393

merged 4 commits into from
Feb 19, 2025

Conversation

lohedges
Copy link
Contributor

This PR fixes #392 by improving the robustness of the internal formal charge calculation within the Parameters sub-module. Now we preferentially use the format the the original molecule was loaded from, either PDB or SDF, or try both if none are present in the file formats list. This means that we can still get a result if the PDB conversion doesn't work, etc.

Note that I haven't been provided with the problem file in this case, so haven't added an additional test. All existing tests pass.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

@lohedges lohedges added bug Something isn't working exscientia Related to work with Exscientia labels Feb 17, 2025
@lohedges lohedges requested a review from mb2055 February 17, 2025 16:08
@lohedges lohedges temporarily deployed to biosimspace-build February 17, 2025 16:08 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build February 17, 2025 16:08 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build February 17, 2025 16:08 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build February 17, 2025 16:08 — with GitHub Actions Inactive
@lohedges
Copy link
Contributor Author

Also note that I chose not to use direct conversion, since it appears to be less reliable if the molecule wasn't loaded from SDF, PDB, or Mol2, i.e. whatever RDKit does to fix things when going from those formats seems to more reliably recover the formal charge than converting directly when that info isn't present, e.g. if you went from a molecule loaded from AMBER files.

Copy link
Contributor

@mb2055 mb2055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good 👍

@lohedges lohedges merged commit 974cdd1 into devel Feb 19, 2025
4 of 5 checks passed
@lohedges lohedges deleted the fix_392 branch February 19, 2025 10:08
lohedges added a commit that referenced this pull request Feb 19, 2025
lohedges added a commit that referenced this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exscientia Related to work with Exscientia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Use direct RDKit conversion when checking formal charge
2 participants