Openff-toolkit: Review and narrow list of OpenEyeToolkitWrapper-supported formats

[j-wags]

Right now, the list of formats that the OpenEye toolkit supports reading is very inclusive:

    _toolkit_file_read_formats = [
        'CAN', 'CDX', 'CSV', 'FASTA', 'INCHI', 'INCHIKEY', 'ISM', 'MDL', 'MF',
        'MMOD', 'MOL2', 'MOL2H', 'MOPAC', 'OEB', 'PDB', 'RDF', 'SDF', 'SKC',
'SLN', 'SMI', 'USM', 'XYC'

Unfortunately, this enables people to parameterize a system from PDB:

from openforcefield.topology import Molecule, Topology
from openforcefield.typing.engines.smirnoff import ForceField
mol = Molecule.from_file('toluene.pdb')
top = Topology.from_molecules(mol)
ff = ForceField('smirnoff99Frosst.offxml')
sys = ff.create_openmm_system(top)

I propose removing all but "CAN", "INCHI", "INCHIKEY", "MDL", "MOL2", "MOL2H", "OEB", "SDF", "SMI" for the time being.

This seems like a pretty urgent fix, so we don't train people to source molecules from dangerous sources.

Any objections? @jchodera @davidlmobley @andrrizzi

[jchodera]

Unfortunately, this enables people to parameterize a system from PDB.

I think this will still use distances to perceive chemical information, so it can work (as long as there are explicit hydrogens!), though it should likely be a matter of last resort.

The big worry is that someone might end up trying to parameterize a system without explicit hydrogens or feeding in molecules without explicit hydrogens to a force field intended to be used with explicit hydrogens. (Note that we should not preclude implicit hydrogen force fields from being developed in future!)

I guess the question is whether we should:

1. exclude any files that do not explicitly contain chemical information, forcing users to do something that may not be "safe" explicitly on their own
2. issue a warning that the chemical perception may not be reliable
3. let them do anything the OpenEye toolkit supports under the hood

[jchodera]

Also, I wouldn't necessarily say this is a "bug" (unless it leads to buggy behavior not reported here), but simply an opportunity to protect users from doing potentially undesirable things. Perhaps there should be a "reliability", "safety", or "sanity checking" label instead?

[j-wags]

exclude any files that do not explicitly contain chemical information, 
forcing users to do something that may not be "safe" explicitly on their own

I'd vote for this method. I'd feel bad requiring people to have a PDB+SDF for their molecules, but really all they need to provide is a PDB+SMILES, so I don't think we're asking too much.

And we can keep the door open for loading from PDB, but we would make it clear that it's not officially supported toolkit functionality. If a person _really_ wants to use a PDB, they could load it into an OEMol and then initialize a molecule like this:

from openforcefield.topology import Molecule, Topology
from openforcefield.typing.engines.smirnoff import ForceField
from openeye import oechem
ifs = oechem.oemolistream('toluene.pdb')
oemol = oechem.OEMol()
oechem.OEReadMolecule(ifs, oemol)
mol = Molecule(oemol)
top = Topology.from_molecules(mol)
ff = ForceField('smirnoff99Frosst.offxml')
sys = ff.create_openmm_system(top)

[jchodera]

This shouldn't be labeled as "api breaking" since it doesn't change the call signatures of any function (though it could cause code that depended on PDB files to create molecule to break, but that's not a large concern at this time since we didn't want to allow that).

[davidlmobley]

In general I agree with this, though I'm in favor of allowing "limited" loading from PDB files -- e.g. if they have a PDB with all protons, adequate CONECT entries, and (a) provided SMILES or (b) provided an override flag of some sort indicating they really wanted to proceed, I'd be OK with proceeding. Note (a) is basically always safe, whereas (b) is... dangerous but sometimes OK if they know what they're getting themselves into.

[j-wags]

Per conversation between @andrrizzi @SimonBoothroyd @karmencj and myself today, and like @davidlmobley suggested -- We should have a trusted-PDB loader. The trusted-PDB loader would ensure that the PDB being read has CONECT records and otherwise lacks ambiguity. This could help us build up to a Topology.from_file method, the lack of which seems to be a major pain point for the property calculator (and likely for normal users too).

[davidlmobley]

Yes. For "well behaved" PDB files this is generally safe, it's just that many novice/academic users do not have well behaved PDB files as a starting point and will try and shove other things through.