Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 27 additions & 17 deletions openfe/tests/protocols/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,23 @@
import pooch


@pytest.fixture(scope='session')
def charged_benzene(benzene_modifications):
benzene_offmol = benzene_modifications['benzene'].to_openff()
benzene_offmol.assign_partial_charges(partial_charge_method='gasteiger')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it pre-charging the molecule that does the speed-up here? If so, can we just include the charges hard-coded in the file, so that we 1) don't need to generate charges at all and 2) remove any reproducibility issues stemming from the partial charge method?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should, but replacing benzene_modifications directly is a bit of a pain because we have tests that check for charges (or lack thereof) that uses these files. So we should do it as a separate data entry (in a separate issue).

Copy link
Member Author

Choose a reason for hiding this comment

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

remove any reproducibility issues stemming from the partial charge method?

There are no reproducibility issues with gasteiger charges. They are fully reproducible, all the time.

Copy link
Member Author

Choose a reason for hiding this comment

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

According to @jthorton it's not if the hybridization changes - so it's onlys table if rdkit doesn't change the graph

return openfe.SmallMoleculeComponent.from_openff(benzene_offmol)

@pytest.fixture
def benzene_vacuum_system(benzene_modifications):
def benzene_vacuum_system(charged_benzene):
return openfe.ChemicalSystem(
{'ligand': benzene_modifications['benzene']},
{'ligand': charged_benzene},
)


@pytest.fixture(scope='session')
def benzene_system(benzene_modifications):
def benzene_system(charged_benzene):
return openfe.ChemicalSystem(
{'ligand': benzene_modifications['benzene'],
{'ligand': charged_benzene,
'solvent': openfe.SolventComponent(
positive_ion='Na', negative_ion='Cl',
ion_concentration=0.15 * unit.molar)
Expand All @@ -29,27 +35,34 @@ def benzene_system(benzene_modifications):


@pytest.fixture
def benzene_complex_system(benzene_modifications, T4_protein_component):
def benzene_complex_system(charged_benzene, T4_protein_component):
return openfe.ChemicalSystem(
{'ligand': benzene_modifications['benzene'],
{'ligand': charged_benzene,
'solvent': openfe.SolventComponent(
positive_ion='Na', negative_ion='Cl',
ion_concentration=0.15 * unit.molar),
'protein': T4_protein_component,}
)


@pytest.fixture(scope='session')
def charged_toluene(benzene_modifications):
offmol = benzene_modifications['toluene'].to_openff()
offmol.assign_partial_charges(partial_charge_method='gasteiger')
return openfe.SmallMoleculeComponent.from_openff(offmol)


@pytest.fixture
def toluene_vacuum_system(benzene_modifications):
def toluene_vacuum_system(charged_toluene):
return openfe.ChemicalSystem(
{'ligand': benzene_modifications['toluene']},
{'ligand': charged_toluene},
)


@pytest.fixture(scope='session')
def toluene_system(benzene_modifications):
def toluene_system(charged_toluene):
return openfe.ChemicalSystem(
{'ligand': benzene_modifications['toluene'],
{'ligand': charged_toluene,
'solvent': openfe.SolventComponent(
positive_ion='Na', negative_ion='Cl',
ion_concentration=0.15 * unit.molar),
Expand All @@ -58,9 +71,9 @@ def toluene_system(benzene_modifications):


@pytest.fixture
def toluene_complex_system(benzene_modifications, T4_protein_component):
def toluene_complex_system(charged_toluene, T4_protein_component):
return openfe.ChemicalSystem(
{'ligand': benzene_modifications['toluene'],
{'ligand': charged_toluene,
'solvent': openfe.SolventComponent(
positive_ion='Na', negative_ion='Cl',
ion_concentration=0.15 * unit.molar),
Expand All @@ -69,13 +82,10 @@ def toluene_complex_system(benzene_modifications, T4_protein_component):


@pytest.fixture(scope='session')
def benzene_to_toluene_mapping(benzene_modifications):
def benzene_to_toluene_mapping(charged_benzene, charged_toluene):
mapper = openfe.setup.LomapAtomMapper(element_change=False)

molA = benzene_modifications['benzene']
molB = benzene_modifications['toluene']

return next(mapper.suggest_mappings(molA, molB))
return next(mapper.suggest_mappings(charged_benzene, charged_toluene))


@pytest.fixture
Expand Down
9 changes: 3 additions & 6 deletions openfe/tests/protocols/test_openmm_equil_rfe_protocols.py
Original file line number Diff line number Diff line change
Expand Up @@ -995,19 +995,16 @@ def test_missing_ligand(benzene_system, benzene_to_toluene_mapping):
)


def test_vaccuum_PME_error(benzene_vacuum_system, benzene_modifications,
benzene_to_toluene_mapping):
# state B doesn't have a solvent component (i.e. its vacuum)
stateB = openfe.ChemicalSystem({'ligand': benzene_modifications['toluene']})

def test_vacuum_PME_error(benzene_vacuum_system, toluene_vacuum_system,
benzene_to_toluene_mapping):
p = openmm_rfe.RelativeHybridTopologyProtocol(
settings=openmm_rfe.RelativeHybridTopologyProtocol.default_settings(),
)
errmsg = "PME cannot be used for vacuum transform"
with pytest.raises(ValueError, match=errmsg):
_ = p.create(
stateA=benzene_vacuum_system,
stateB=stateB,
stateB=toluene_vacuum_system,
mapping=benzene_to_toluene_mapping,
)

Expand Down
7 changes: 4 additions & 3 deletions openfe/tests/protocols/test_openmmutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,9 @@ def get_settings():

class TestFEAnalysis:

# Note: class scope _will_ cause this to segfault - the reporter has to close
@pytest.fixture(scope='function')
# Note: scope on this can sometimes cause segfault, may need to revert to
# function scope if it happens.
@pytest.fixture(scope='class')
Copy link
Contributor

Choose a reason for hiding this comment

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

this is purely for speed-up reasons? I'm hesitant about increasing test fixture scope as a shortcut if we're at all concerned about them being mutated.

Copy link
Member Author

Choose a reason for hiding this comment

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

(for the future) we discussed this and agreed that we should remove session scopes on everything.

def reporter(self):
with resources.files('openfe.tests.data.openmm_rfe') as d:
ncfile = str(d / 'vacuum_nocoord.nc')
Expand All @@ -244,7 +245,7 @@ def reporter(self):
finally:
r.close()

@pytest.fixture()
@pytest.fixture(scope='class')
Copy link
Contributor

Choose a reason for hiding this comment

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

same question as above

Copy link
Member Author

Choose a reason for hiding this comment

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

(for the future) this one really benefits from a session scope, we'll try to leave it here for now.

def analyzer(self, reporter):
return multistate_analysis.MultistateEquilFEAnalysis(
reporter, sampling_method='repex',
Expand Down