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

Refactor to openff.units.elements #1182

Merged
merged 12 commits into from
Feb 10, 2022
1 change: 0 additions & 1 deletion devtools/conda-envs/beta_rc_env.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ dependencies:
- networkx
- ambertools
- rdkit
- mendeleev
- xmltodict
# Test-only/optional/dev
- mdtraj
Expand Down
6 changes: 3 additions & 3 deletions devtools/conda-envs/openeye.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,14 @@ dependencies:
- coverage
- numpy
- networkx
- parmed # ???
- mendeleev
- parmed # ??? what ???
Copy link
Member

Choose a reason for hiding this comment

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

(not blocking) Maybe this was for example tests (from before they were separated into their own CI)? Feel free to try removing if you have time to let CI run.

- openeye-toolkits
- packaging
# Removed until a new release which targets openff-toolkit is made.
# - openmmforcefields
- openmm
- openff-forcefields
- openff-units
- openff-units >=0.1.5
- openff-utilities
- smirnoff99Frosst
- cachetools
Expand All @@ -38,5 +37,6 @@ dependencies:
- qcportal
- qcengine
- mdtraj
- openff-interchange
- pip:
- git+https://github.com/openforcefield/openff-interchange.git@v0.2.0-alpha.1
3 changes: 1 addition & 2 deletions devtools/conda-envs/rdkit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,12 @@ dependencies:
- networkx
- ambertools
- rdkit
- mendeleev
- packaging
# Removed until a new release which targets openff-toolkit is made.
# - openmmforcefields
- openmm
- openff-forcefields
- openff-units >=0.1.4
- openff-units >=0.1.5
- openff-utilities
- smirnoff99Frosst
- cachetools
Expand Down
4 changes: 1 addition & 3 deletions devtools/conda-envs/test_env.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,14 @@ dependencies:
- packaging
- openmm >=7.6
- openff-forcefields
- openff-units >=0.1.4
- openff-units >=0.1.5
- openff-utilities
- smirnoff99Frosst
- numpy
- networkx
- ambertools
- rdkit
- mendeleev
- xmltodict
- mendeleev
# Test-only/optional/dev
- pytest
- pytest-cov
Expand Down
76 changes: 69 additions & 7 deletions docs/releasehistory.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,90 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w
* `minor` increments add features but do not break API compatibility
* `micro` increments represent bugfix releases or improvements in documentation

## Migration guide

### Units

Import the [unit registry](https://pint.readthedocs.io/en/stable/developers_reference.html?highlight=unitregistry#pint.UnitRegistry) provided by `openff-units`:

```
from openff.units import unit
```

Create a `unit.Quantity` object:
```
value = unit.Quantity(1.0, unit.nanometer) # or 1.0 * unit.nanometer
```

Inspect the value and unit of this quantity:
```
print(value.magnitude) # or value.m
# 1.0
print(value.units)
# <Unit('nanometer')>
```

Convert to compatible units:
```
converted = value.to(unit.angstrom)
print(converted)
# 10.0 <Unit('angstrom')>
```

Report the value in compatible units:
```
print(value.magnitude_as(unit.angstrom)) $ or .m_as()
# 10.0 <Unit('angstrom')>
```

Convert to and from OpenMM quantities:
```
from openff.units.openmm import to_openmm, from_openmm
value_openmm = to_openmm(value)
print(value_openmm)
# Quantity(value=1.0, unit=nanometer)
print(type(value_openmm))
# 1.0 <Unit('nanometer')>
value_roundtrip = from_openmm(value_openmm)
print(value_roundtrip)
# 1.0 <Unit('nanometer')>
```


## Current Development

- [PR #964](https://github.com/openforcefield/openff-toolkit/pull/964): Adds initial implementation
of atom metadata dictionaries.
- [PR #1097](https://github.com/openforcefield/openff-toolkit/pull/1097): Deprecates TopologyMolecule.
- [PR #1097](https://github.com/openforcefield/openff-toolkit/pull/1097): Topology.from_openmm
is no longer guaranteed to maintain the ordering of bonds, but now explicitly guarantees that it maintains
the order of atoms (Neither of these ordering guarantees were explicitly documented before, but this may be a
change from the previous behavior).
of atom metadata dictionaries.
- [PR #1097](https://github.com/openforcefield/openff-toolkit/pull/1097): Deprecates TopologyMolecule.
- [PR #1097](https://github.com/openforcefield/openff-toolkit/pull/1097): Topology.from_openmm
is no longer guaranteed to maintain the ordering of bonds, but now explicitly guarantees that it maintains
the order of atoms (Neither of these ordering guarantees were explicitly documented before, but this may be a
change from the previous behavior).
- [PR #1165](https://github.com/openforcefield/openforcefield/pull/1165): Adds the boolean argument
`use_interchange` to
[`create_openmm_system`](openff.toolkit.typing.engines.smirnoff.ForceField.create_openmm_system)
with a default value of False. Setting it to True routes `openmm.System` creation through
Interchange.

### Behaviors changed and bugfixes

- [PR #1182](https://github.com/openforcefield/openforcefield/pull/1182) and
[PR #1142](https://github.com/openforcefield/openforcefield/pull/1142) migrate handling of
unit-bearing quantities from OpenMM's unit module (`openmm.unit`) to
[`openff-units`](https://github.com/openforcefield/openff-units), which itself is based off of
[Pint](https://pint.readthedocs.io/en/stable/).
- [PR #1118](https://github.com/openforcefield/openforcefield/pull/1118):
[`Molecule.to_hill_formula`](openff.toolkit.topology.Molecule.to_hill_formula) is now a class method
and no longer accepts input of NetworkX graphs.
- [PR #1130](https://github.com/openforcefield/openforcefield/pull/1130): Running unit tests will
no longer generate force field files in the local directory.
- [PR #1182](https://github.com/openforcefield/openforcefield/pull/1182): Removes `Atom.element`,
thereby also removing `Atom.element.symbol`, `Atom.element.mass` and `Atom.element.atomic_number`.
These are replaced with corresponding properties directly on the
[`Atom`](openff.toolkit.topology.molecule.Atom) class:
[`Atom.symbol`](openff.toolkit.topology.molecule.Atom.symbol),
[`Atom.mass`](openff.toolkit.topology.molecule.Atom.mass), and
[`Atom.atomic_number`](openff.toolkit.topology.molecule.Atom.atomic_number).

### Examples added

Expand Down
8 changes: 4 additions & 4 deletions openff/toolkit/tests/test_forcefield.py
Original file line number Diff line number Diff line change
Expand Up @@ -3291,8 +3291,8 @@ def check_molecule_constraints(cls, molecule, system, bond_elements, bond_length
constraint_idx
)
atom_elements = {
molecule.atoms[atom1_idx].element.symbol,
molecule.atoms[atom2_idx].element.symbol,
molecule.atoms[atom1_idx].symbol,
molecule.atoms[atom2_idx].symbol,
}
assert atom_elements == bond_elements
assert np.isclose(
Expand Down Expand Up @@ -3635,7 +3635,7 @@ def test_freesolv_gbsa_energies(

# Give each atom a unique name, otherwise OpenMM will complain
for idx, atom in enumerate(molecule.atoms):
atom.name = f"{atom.element.symbol}{idx}"
atom.name = f"{atom.symbol}{idx}"
positions = to_openmm(molecule.conformers[0])

off_gbsas = {
Expand Down Expand Up @@ -3939,7 +3939,7 @@ def test_molecule_energy_gb_no_sa(self, zero_charges, gbsa_model):

# Give each atom a unique name, otherwise OpenMM will complain
for idx, atom in enumerate(molecule.atoms):
atom.name = f"{atom.element.symbol}{idx}"
atom.name = f"{atom.symbol}{idx}"

positions = np.concatenate(
(molecule.conformers[0], molecule.conformers[0] + (10 * unit.angstrom))
Expand Down
28 changes: 14 additions & 14 deletions openff/toolkit/tests/test_molecule.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@

import numpy as np
import pytest
from mendeleev import element
from openff.units import unit
from openff.units.elements import MASSES, SYMBOLS
from openmm import unit as openmm_unit

from openff.toolkit.tests.create_molecules import (
Expand Down Expand Up @@ -302,28 +302,28 @@ def test_atom_constructor(self):
atom1 = Atom(6, 1 * unit.elementary_charge, False)
assert atom1.formal_charge == 1 * unit.elementary_charge

@pytest.mark.parametrize("atomic_number", range(1, 118))
@pytest.mark.parametrize("atomic_number", range(1, 117))
def test_atom_properties(self, atomic_number):
"""Test that atom properties are correctly populated and gettable"""
formal_charge = 0 * unit.elementary_charge
is_aromatic = False

this_element = element(atomic_number)
expected_mass = MASSES[atomic_number]
expected_symbol = SYMBOLS[atomic_number]

atom = Atom(
this_element.atomic_number,
atomic_number,
formal_charge,
is_aromatic,
name=this_element.name,
name="fOO",
)
assert atom.atomic_number == this_element.atomic_number
# Equality comparison fails with mendeleev v0.9.0 and older,
# should be fixed when a new release is made
# assert atom.element == this_element
assert atom.atomic_number == atomic_number
assert atom.formal_charge == formal_charge
assert atom.is_aromatic == is_aromatic
assert atom.name == this_element.name
assert atom.mass == this_element.mass
assert atom.symbol == expected_symbol
assert atom.mass == expected_mass
mattwthompson marked this conversation as resolved.
Show resolved Hide resolved
assert atom.mass.units == unit.dalton
assert atom.name == "fOO"

def test_atom_metadata(self):
"""Test that atom metadata behaves as expected"""
Expand Down Expand Up @@ -774,15 +774,15 @@ def test_partial_mapped_smiles(self, toolkit_class, data):
# now we just need to check the smiles generated
if data["atom_map"] is None:
for i, atom in enumerate(mol.atoms, 1):
assert f"[{atom.element.symbol}:{i}]" in smiles
assert f"[{atom.symbol}:{i}]" in smiles
else:
if 0 in data["atom_map"].values():
increment = True
else:
increment = False

for atom, index in data["atom_map"].items():
assert f"[{mol.atoms[atom].element.symbol}:{index + 1 if increment else index}]"
assert f"[{mol.atoms[atom].symbol}:{index + 1 if increment else index}]"

else:
pytest.skip(
Expand Down Expand Up @@ -1532,7 +1532,7 @@ def test_strip_atom_stereochemistry(self):
mol = Molecule.from_smiles("CCC[N@@](C)CC")

nitrogen_idx = [
atom.molecule_atom_index for atom in mol.atoms if atom.element.symbol == "N"
atom.molecule_atom_index for atom in mol.atoms if atom.symbol == "N"
][0]

# TODO: This fails with RDKitToolkitWrapper because it perceives
Expand Down
26 changes: 12 additions & 14 deletions openff/toolkit/tests/test_topology.py
Original file line number Diff line number Diff line change
Expand Up @@ -266,24 +266,22 @@ def test_get_atom(self, ethane_from_smiles):
with pytest.raises(Exception) as context:
topology_atom = topology.atom(8)

def test_topology_atom_element(self, toluene_from_sdf):
"""Test getters of TopologyAtom element and atomic number"""
from mendeleev import element
from mendeleev.models import Element

def test_topology_atom_element_properties(self, toluene_from_sdf):
"""
Test element-like getters of TopologyAtom atomic number. In 0.11.0, Atom.element
was removed and replaced with Atom.atomic_number and Atom.symbol.
"""
topology = Topology()
topology.add_molecule(toluene_from_sdf)

first_element = topology.atom(0).element
eighth_element = topology.atom(7).element

# Check if types/instances are correct
assert isinstance(first_element, Element)
assert isinstance(eighth_element, Element)
first_atom = topology.atom(0)
eighth_atom = topology.atom(7)

# Make sure first is a carbon element and eighth is a hydrogen element
assert first_element.symbol == element(6).symbol
assert eighth_element.symbol == element(1).symbol
# These atoms are expected to be hydrogen and carbon, respectively
assert first_atom.symbol == "C"
assert first_atom.atomic_number == 6
assert eighth_atom.symbol == "H"
assert eighth_atom.atomic_number == 1

def test_get_bond(self, ethane_from_smiles, ethene_from_smiles):
"""Test Topology.bond function (bond lookup from index)"""
Expand Down
Loading