Skip to content
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

Raise exception when processing molecule with radical electrons #1236

Merged
merged 17 commits into from
Sep 23, 2022

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Mar 29, 2022

Here's a block to add to the changelog, keeping it out of the tree to make merges easier:

- [PR #1236](https://github.com/openforcefield/openff-toolkit/pull/1236) raises
  [`RadicalsNotSupportedError`](openff.toolkit.utils.exceptions.RadicalsNotSupportedError) when
  [`Molecule.from_openeye`](openff.toolkit.topology.Molecule.from_openeye),
  [`Molecule.from_rdkit`](openff.toolkit.topology.Molecule.from_rdkit),
  [`Molecule.from_smiles`](openff.toolkit.topology.Molecule.from_smiles), or similar methods
  detect that a molecule contains radical electrons.

Resolves #1075, assuming I haven't forgotten how chemistry works.

This is a follow-on to #1098 targeting the v0.11.x line.

Two remaining considerations may be:

  • Performance evaluation - this adds an O(number of atoms) check to two key conversion methods.
  • False positives - I have no idea if this will be too noisy and claim to have found radicals where none exist. Throwing this at a large dataset should give an indication of this.

commit 28c1923c23749b5938add197d4b964afd67ae051
Merge: 3c5454e 921fbe2
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Tue Mar 29 14:21:28 2022 -0500

    Merge remote-tracking branch 'upstream/master' into no-radicals

commit 3c5454e
Author: Matthew W. Thompson <mattwthompson@protonmail.com>
Date:   Wed Sep 29 13:28:27 2021 -0500

    Raise exception when RDKit finds a SMILES/rdmol with radical(s)
@mattwthompson mattwthompson added this to the 0.11.1 milestone Mar 29, 2022
@openforcefield openforcefield deleted a comment from lgtm-com bot Mar 29, 2022
@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #1236 (63d2a48) into main (3bdc38f) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head 63d2a48 differs from pull request most recent head 6dc41ca. Consider uploading reports for the commit 6dc41ca to get more accurate results

Additional details and impacted files

@mattwthompson
Copy link
Member Author

Some InChi roundtrips are showing different behavior; I don't know if that's good or bad.

I've added a milestone for 0.11.1 because this is not a requirement for the 0.11.0 release.

@mattwthompson mattwthompson changed the base branch from topology-biopolymer-refactor to main July 1, 2022 18:24
@j-wags
Copy link
Member

j-wags commented Sep 16, 2022

I'll look into these test failures, but to recap a statement I just made in a synchronous meeting with @mattwthompson:

There are three outcomes to a user loading a molecule with a radical into OFFTK:

  1. The molecule could be loaded with a cheminformatics toolkit "fixing" the radical by adding an H
  2. The user could get an error
  3. The user could get an OFFMol with a radical

Right now, trying to parameterize neutral CH3 gets us errors from quacpac and antechamber, so that's clearly an invalid input for our infrastructure. CCCC[S+] emitted an antechamber warning, but succeeded in parameterization and resulted in all-0 partial charges (!).

So, I'm coming around to the idea of totally forbidding radicals for now in the toolkit, at least until we have defined behavior specified, and we establish that we explicitly DO aim to have those in scope.

@j-wags
Copy link
Member

j-wags commented Sep 21, 2022

Allllright, I think this should be fixed throughout but I'll wait for tests to pass before running a final review. Minidrugbank is such an awful test set, maybe things are finally quiet enough that I can sit down and make a new one.

@mattwthompson
Copy link
Member Author

Humor shows up in unexpected places:

=================================== FAILURES ===================================
_ examples/check_dataset_parameter_coverage/check_parameter_coverage.ipynb::Cell 2 _
Notebook cell execution failed
Cell 2: Cell execution caused an exception

Input:
molecules = Molecule.from_file("example_molecules.smi", allow_undefined_stereo=True)

# We also provide a SMILES dataset of ~1000 problematic molecules
# molecules =  Molecule.from_file('problem_smiles.smi', allow_undefined_stereo=True)

print(f"Loaded {len(molecules)} molecules")

Traceback:

---------------------------------------------------------------------------
RadicalsNotSupportedError                 Traceback (most recent call last)
Cell In [4], line 1
----> 1 molecules = Molecule.from_file("example_molecules.smi",allow_undefined_stereo=True)
      3 # We also provide a SMILES dataset of ~1000 problematic molecules
      4 # molecules =  Molecule.from_file('problem_smiles.smi', allow_undefined_stereo=True)
      6 print(f"Loaded {len(molecules)} molecules")

…ut failing to parameterize a radical, which now instead throws an error immediately upon loading
@j-wags
Copy link
Member

j-wags commented Sep 23, 2022

Humor shows up in unexpected places:

Laughing it up all the way to the deprecated folder. I don't feel like rewriting a large fraction of that notebook, and it was largely a early marketing tool to show groups "hey look, we COULD parameterize your molecules". We have more impressive things to show off now.

@j-wags j-wags merged commit ed6549e into main Sep 23, 2022
@j-wags j-wags deleted the no-radicals-2 branch September 23, 2022 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should the Molecule class support radicals?
2 participants