[WIP] Find rotatable bonds #506
Conversation
jthorton
commented
Feb 6, 2020
•
jthorton
commented
- Fixes Include find rotatable bonds in OETKW and RDKTKW #500
- Fixes OFFMol.to_rdkit loses aromaticity info #513
- Add tests
- Update docstrings/documentation, if applicable
- Update changelog
… __str__ methods for the Bonds class. Tests added for the new method.
|
This is the my initial go at adding the ability to find rotatable bonds, which seems to work quite well. I have not added a new bond attribute def find_rotatable_bonds(self, ignore_functional_groups=None, toolkit_registry=GLOBAL_TOOLKIT_REGISTRY):
rotatable_bond_smarts = '[!$(*#*)&!D1:1]-&!@[!$(*#*)&!D1:2]'
# get all of the general matches
general_matches = self.chemical_environment_matches(query=rotatable_bond_smarts,
toolkit_registry=toolkit_registry)
ignore_functional_groups = [list of SMARTS to ignore]
ignore_bonds = toolkit_registryfind_smarts_matches to find matches for the items in
ignore_functional_groups
Remove ignore_bonds from rotatable_bonds
# now reset the is_rotatable attribute on each bond to match the search
for bond in rotatbale_bonds:
bond.is_rotatable = True
return rotatable_bonds |
Conflicts: openforcefield/tests/test_molecule.py
| def __repr__(self): | ||
| return f"Bond(atom1 index={self.atom1_index}, atom2 index={self.atom2_index})" | ||
|
|
||
| def __str__(self): | ||
| return f"<Bond atom1 index='{self.atom1_index}', atom2 index='{self.atom2_index}'>" | ||
|
|
| def condense_matches(matches): | ||
| condensed_matches = set() | ||
| for m in matches: | ||
| condensed_matches.add(tuple(sorted(m))) | ||
| return condensed_matches | ||
|
|
||
| general_bonds = condense_matches(general_matches) |
| # now ignore the C-O bond | ||
| bonds = ethanol.find_rotatable_bonds('[#6:1]-[#8:2]') | ||
| assert len(bonds) == 1 | ||
| assert ethanol.atoms[bonds[0].atom1_index].atomic_number == 6 | ||
| assert ethanol.atoms[bonds[0].atom2_index].atomic_number == 6 |
There was a problem hiding this comment.
Could you copy this block and also run the check when the SMIRKS indices are reversed? The sorted line in the new function should handle it, but it's always good to be sure.
There was a problem hiding this comment.
Good point, I have added that as well.
openforcefield/topology/molecule.py
Outdated
| if not isinstance(ignore_functional_groups, list): | ||
| ignore_functional_groups = [ignore_functional_groups] |
There was a problem hiding this comment.
Since there are iterable objects other than lists, we try to follow a design pattern like this:
There was a problem hiding this comment.
Thanks for catching that that only problem is if the user gives a single SMIRKS string this is iterable which would cause issues so maybe if I type check for a str first and then do this it would work?
| def test_find_rotatable_bonds(self): | ||
| """Test finding rotatable bonds while ignoring some groups""" |
There was a problem hiding this comment.
Could you move this to test_toolkits, and run once with OETK and another time with RDKit? I know the different setups are technically covered in our Travis testing matrix, but weird things can happen when both toolkits are installed that hide sneaky problems.
There was a problem hiding this comment.
No problem its best to be safe here, I have moved the tests.
