From 04af8ab5d0f3cc277efb98905d26c819ed35a7f9 Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Fri, 14 Feb 2025 19:03:14 +0000 Subject: [PATCH 01/11] Switch some tests to already have charges --- openfe/tests/protocols/conftest.py | 44 ++++++++++++++++++------------ 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/openfe/tests/protocols/conftest.py b/openfe/tests/protocols/conftest.py index 743352215..aecd235ff 100644 --- a/openfe/tests/protocols/conftest.py +++ b/openfe/tests/protocols/conftest.py @@ -11,16 +11,22 @@ @pytest.fixture -def benzene_vacuum_system(benzene_modifications): +def charged_benzene(benzene_modifications): + benzene_offmol = benzene_modifications['benzene'].to_openff() + benzene_offmol.assign_partial_charges(partial_charge_method='gasteiger') + return openfe.SmallMoleculeComponent.from_openff(benzene_offmol) + +@pytest.fixture +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) @@ -29,9 +35,9 @@ 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), @@ -40,16 +46,23 @@ def benzene_complex_system(benzene_modifications, T4_protein_component): @pytest.fixture -def toluene_vacuum_system(benzene_modifications): +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(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), @@ -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), @@ -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 From 59d3336d3ad3e4dbf4c57ce5f88da0819515b69c Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Fri, 14 Feb 2025 19:18:29 +0000 Subject: [PATCH 02/11] Fix the scope mismatch issue --- openfe/tests/protocols/conftest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openfe/tests/protocols/conftest.py b/openfe/tests/protocols/conftest.py index aecd235ff..2c7270870 100644 --- a/openfe/tests/protocols/conftest.py +++ b/openfe/tests/protocols/conftest.py @@ -10,7 +10,7 @@ import pooch -@pytest.fixture +@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') @@ -45,7 +45,7 @@ def benzene_complex_system(charged_benzene, T4_protein_component): ) -@pytest.fixture +@pytest.fixture(scope='session') def charged_toluene(benzene_modifications): offmol = benzene_modifications['toluene'].to_openff() offmol.assign_partial_charges(partial_charge_method='gasteiger') From 642096cdfd2b5011f629a98aa7e3ddf8ce7b95e4 Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Fri, 14 Feb 2025 19:39:16 +0000 Subject: [PATCH 03/11] fix that one test --- openfe/tests/protocols/test_openmm_equil_rfe_protocols.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py index 513962355..bdbf7ed64 100644 --- a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py +++ b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py @@ -995,8 +995,8 @@ def test_missing_ligand(benzene_system, benzene_to_toluene_mapping): ) -def test_vaccuum_PME_error(benzene_vacuum_system, benzene_modifications, - benzene_to_toluene_mapping): +def test_vacuum_PME_error(benzene_vacuum_system, toluene_vacuum_system, + benzene_to_toluene_mapping): # state B doesn't have a solvent component (i.e. its vacuum) stateB = openfe.ChemicalSystem({'ligand': benzene_modifications['toluene']}) From a005cab82c64694bb6fc9aa3fa961709d5f416b9 Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Fri, 14 Feb 2025 19:43:07 +0000 Subject: [PATCH 04/11] try speeding up FEAnalysis --- openfe/tests/protocols/test_openmmutils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openfe/tests/protocols/test_openmmutils.py b/openfe/tests/protocols/test_openmmutils.py index 2664c0011..9944429aa 100644 --- a/openfe/tests/protocols/test_openmmutils.py +++ b/openfe/tests/protocols/test_openmmutils.py @@ -228,7 +228,7 @@ def get_settings(): class TestFEAnalysis: # Note: class scope _will_ cause this to segfault - the reporter has to close - @pytest.fixture(scope='function') + @pytest.fixture(scope='class') def reporter(self): with resources.files('openfe.tests.data.openmm_rfe') as d: ncfile = str(d / 'vacuum_nocoord.nc') @@ -244,7 +244,7 @@ def reporter(self): finally: r.close() - @pytest.fixture() + @pytest.fixture(scope='class') def analyzer(self, reporter): return multistate_analysis.MultistateEquilFEAnalysis( reporter, sampling_method='repex', From 8027511781d37c8d679cc6a2e259d646f7ef25e3 Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Fri, 14 Feb 2025 19:57:35 +0000 Subject: [PATCH 05/11] fix test and update comment --- openfe/tests/protocols/test_openmm_equil_rfe_protocols.py | 2 +- openfe/tests/protocols/test_openmmutils.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py index bdbf7ed64..af737d5ec 100644 --- a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py +++ b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py @@ -998,7 +998,7 @@ def test_missing_ligand(benzene_system, benzene_to_toluene_mapping): def test_vacuum_PME_error(benzene_vacuum_system, toluene_vacuum_system, benzene_to_toluene_mapping): # state B doesn't have a solvent component (i.e. its vacuum) - stateB = openfe.ChemicalSystem({'ligand': benzene_modifications['toluene']}) + stateB = openfe.ChemicalSystem({'ligand': toluene_vacuum_system}) p = openmm_rfe.RelativeHybridTopologyProtocol( settings=openmm_rfe.RelativeHybridTopologyProtocol.default_settings(), diff --git a/openfe/tests/protocols/test_openmmutils.py b/openfe/tests/protocols/test_openmmutils.py index 9944429aa..5da60333f 100644 --- a/openfe/tests/protocols/test_openmmutils.py +++ b/openfe/tests/protocols/test_openmmutils.py @@ -227,7 +227,8 @@ def get_settings(): class TestFEAnalysis: - # Note: class scope _will_ cause this to segfault - the reporter has to close + # Note: scope on this can sometimes cause segfault, may need to revert to + # function scope if it happens. @pytest.fixture(scope='class') def reporter(self): with resources.files('openfe.tests.data.openmm_rfe') as d: From 72b99f3b67c860bed6748a4e570dfb0abc2b056a Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Fri, 14 Feb 2025 20:17:07 +0000 Subject: [PATCH 06/11] Fix that test finally --- openfe/tests/protocols/test_openmm_equil_rfe_protocols.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py index af737d5ec..7b5de012d 100644 --- a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py +++ b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py @@ -997,9 +997,6 @@ def test_missing_ligand(benzene_system, benzene_to_toluene_mapping): def test_vacuum_PME_error(benzene_vacuum_system, toluene_vacuum_system, benzene_to_toluene_mapping): - # state B doesn't have a solvent component (i.e. its vacuum) - stateB = openfe.ChemicalSystem({'ligand': toluene_vacuum_system}) - p = openmm_rfe.RelativeHybridTopologyProtocol( settings=openmm_rfe.RelativeHybridTopologyProtocol.default_settings(), ) @@ -1007,7 +1004,7 @@ def test_vacuum_PME_error(benzene_vacuum_system, toluene_vacuum_system, with pytest.raises(ValueError, match=errmsg): _ = p.create( stateA=benzene_vacuum_system, - stateB=stateB, + stateB=toluene_vacuum_system, mapping=benzene_to_toluene_mapping, ) From 73c199fac408503c117ee32f35532776c37c53f5 Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Mon, 17 Feb 2025 15:46:28 +0000 Subject: [PATCH 07/11] Remove some session scoped things --- openfe/tests/protocols/conftest.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/openfe/tests/protocols/conftest.py b/openfe/tests/protocols/conftest.py index 2c7270870..fca93af7f 100644 --- a/openfe/tests/protocols/conftest.py +++ b/openfe/tests/protocols/conftest.py @@ -10,7 +10,7 @@ import pooch -@pytest.fixture(scope='session') +@pytest.fixture def charged_benzene(benzene_modifications): benzene_offmol = benzene_modifications['benzene'].to_openff() benzene_offmol.assign_partial_charges(partial_charge_method='gasteiger') @@ -23,7 +23,7 @@ def benzene_vacuum_system(charged_benzene): ) -@pytest.fixture(scope='session') +@pytest.fixture def benzene_system(charged_benzene): return openfe.ChemicalSystem( {'ligand': charged_benzene, @@ -45,7 +45,7 @@ def benzene_complex_system(charged_benzene, T4_protein_component): ) -@pytest.fixture(scope='session') +@pytest.fixture def charged_toluene(benzene_modifications): offmol = benzene_modifications['toluene'].to_openff() offmol.assign_partial_charges(partial_charge_method='gasteiger') @@ -59,7 +59,7 @@ def toluene_vacuum_system(charged_toluene): ) -@pytest.fixture(scope='session') +@pytest.fixture def toluene_system(charged_toluene): return openfe.ChemicalSystem( {'ligand': charged_toluene, @@ -81,10 +81,9 @@ def toluene_complex_system(charged_toluene, T4_protein_component): ) -@pytest.fixture(scope='session') +@pytest.fixture def benzene_to_toluene_mapping(charged_benzene, charged_toluene): mapper = openfe.setup.LomapAtomMapper(element_change=False) - return next(mapper.suggest_mappings(charged_benzene, charged_toluene)) From 967f01ab9cb11c3d5c904f22813ec3df5c5581ef Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Mon, 17 Feb 2025 16:32:32 +0000 Subject: [PATCH 08/11] try to fix the session scope here --- openfe/tests/protocols/conftest.py | 1 + .../test_openmm_equil_rfe_protocols.py | 39 +++++++++++++++---- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/openfe/tests/protocols/conftest.py b/openfe/tests/protocols/conftest.py index fca93af7f..a795dd486 100644 --- a/openfe/tests/protocols/conftest.py +++ b/openfe/tests/protocols/conftest.py @@ -16,6 +16,7 @@ def charged_benzene(benzene_modifications): benzene_offmol.assign_partial_charges(partial_charge_method='gasteiger') return openfe.SmallMoleculeComponent.from_openff(benzene_offmol) + @pytest.fixture def benzene_vacuum_system(charged_benzene): return openfe.ChemicalSystem( diff --git a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py index 7b5de012d..7b49d325c 100644 --- a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py +++ b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py @@ -426,8 +426,11 @@ def test_confgen_mocked_fail(benzene_system, toluene_system, protocol = openmm_rfe.RelativeHybridTopologyProtocol(settings=settings) - dag = protocol.create(stateA=benzene_system, stateB=toluene_system, - mapping=benzene_to_toluene_mapping) + dag = protocol.create( + stateA=benzene_system, + stateB=toluene_system, + mapping=benzene_to_toluene_mapping, + ) dag_unit = list(dag.protocol_units)[0] with tmpdir.as_cwd(): @@ -439,12 +442,34 @@ def test_confgen_mocked_fail(benzene_system, toluene_system, @pytest.fixture(scope='session') def tip4p_hybrid_factory( - benzene_system, toluene_system, - benzene_to_toluene_mapping, tmp_path_factory, + benzene_modifications, tmp_path_factory, ): """ Hybrid system with virtual sites in the environment (waters) """ + # Session scoped, so we do things by hand here + # Generate the end state systems + benzene_offmol = benzene_modifications['benzene'].to_openff() + benzene_offmol.assign_partial_charges(partial_charge_method='gasteiger') + benzene = openfe.SmallMoleculeComponent.from_openff(benzene_offmol) + + toluene_offmol = benzene_modifications['toluene'].to_openff() + toluene_offmol.assign_partial_charges(partial_charge_method='gasteiger') + toluene = openfe.SmallMoleculeComponent.from_openff(toluene_offmol) + + solvent = openfe.SolventComponent( + positive_ion='Na', + negative_ion='Cl', + ion_concentration=0.15 * unit.molar + ) + + stateA = openfe.ChemicalSystem({'ligand': benzene, 'solvent': solvent}) + stateB = openfe.ChemicalSystem({'ligand': toluene, 'solvent': solvent}) + + # Now the mapping + mapper = openfe.setup.LomapAtomMapper(element_change=False) + mapping = next(mapper.suggest_mappings(charged_benzene, charged_toluene)) + settings = openmm_rfe.RelativeHybridTopologyProtocol.default_settings() settings.forcefield_settings.forcefields = [ "amber/ff14SB.xml", # ff14SB protein force field @@ -460,9 +485,9 @@ def tip4p_hybrid_factory( settings=settings, ) dag = protocol.create( - stateA=benzene_system, - stateB=toluene_system, - mapping=benzene_to_toluene_mapping, + stateA=stateA, + stateB=stateB, + mapping=mapping, ) dag_unit = list(dag.protocol_units)[0] From ab46a8ac1b55d6133ad035c4a63de31b9cc9bf17 Mon Sep 17 00:00:00 2001 From: Irfan Alibay Date: Mon, 17 Feb 2025 16:47:24 +0000 Subject: [PATCH 09/11] fix typo --- openfe/tests/protocols/test_openmm_equil_rfe_protocols.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py index 7b49d325c..af20b9b73 100644 --- a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py +++ b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py @@ -468,7 +468,7 @@ def tip4p_hybrid_factory( # Now the mapping mapper = openfe.setup.LomapAtomMapper(element_change=False) - mapping = next(mapper.suggest_mappings(charged_benzene, charged_toluene)) + mapping = next(mapper.suggest_mappings(benzene, toluene)) settings = openmm_rfe.RelativeHybridTopologyProtocol.default_settings() settings.forcefield_settings.forcefields = [ From fff615b488fcbd85d414041e33172b4769db11da Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Tue, 18 Feb 2025 12:53:32 -0800 Subject: [PATCH 10/11] fixing typo --- openfe/protocols/openmm_rfe/equil_rfe_methods.py | 2 +- openfe/tests/protocols/test_openmm_equil_rfe_protocols.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/openfe/protocols/openmm_rfe/equil_rfe_methods.py b/openfe/protocols/openmm_rfe/equil_rfe_methods.py index d2f95f2c3..4668a89d2 100644 --- a/openfe/protocols/openmm_rfe/equil_rfe_methods.py +++ b/openfe/protocols/openmm_rfe/equil_rfe_methods.py @@ -188,7 +188,7 @@ def _validate_alchemical_components( Parameters ---------- alchemical_components : dict[str, list[Component]] - Dictionary contatining the alchemical components for + Dictionary containing the alchemical components for states A and B. mapping : Optional[Union[ComponentMapping, list[ComponentMapping]]] all mappings between transforming components. diff --git a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py index af20b9b73..e17d3074a 100644 --- a/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py +++ b/openfe/tests/protocols/test_openmm_equil_rfe_protocols.py @@ -252,6 +252,7 @@ def test_dry_run_gaff_vacuum(benzene_vacuum_system, toluene_vacuum_system, @pytest.mark.slow +## TODO: this is breaking because the components are not the same gufe tokenizables as in the mapping def test_dry_many_molecules_solvent( benzene_many_solv_system, toluene_many_solv_system, benzene_to_toluene_mapping, tmpdir From 4137d167c40bed21f38a53bd0745e6845d1634ab Mon Sep 17 00:00:00 2001 From: IAlibay Date: Tue, 17 Jun 2025 14:35:47 +0100 Subject: [PATCH 11/11] try something --- openfe/tests/conftest.py | 14 ++++++++++++++ openfe/tests/protocols/conftest.py | 16 ++++++++-------- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/openfe/tests/conftest.py b/openfe/tests/conftest.py index f03074ee4..947e5d577 100644 --- a/openfe/tests/conftest.py +++ b/openfe/tests/conftest.py @@ -6,6 +6,7 @@ from importlib import resources from rdkit import Chem from rdkit.Chem import AllChem +from openff.toolkit import Molecule from openff.units import unit import urllib.request @@ -212,6 +213,19 @@ def benzene_modifications(): return files +@pytest.fixture(scope='session') +def charged_benzene_modifications(): + files = {} + with importlib.resources.files('openfe.tests.data') as d: + fn = str(d / 'benzene_modifications.sdf') + supp = Chem.SDMolSupplier(str(fn), removeHs=False) + for rdmol in supp: + offmol = Molecule.from_rdkit(rdmol) + offmol.assign_partial_charges(partial_charge_method='gasteiger') + files[rdmol.GetProp('_Name')] = SmallMoleculeComponent.from_openff(offmol) + return files + + @pytest.fixture def serialization_template(): def inner(filename): diff --git a/openfe/tests/protocols/conftest.py b/openfe/tests/protocols/conftest.py index e8e9f2d2c..afefbb5e3 100644 --- a/openfe/tests/protocols/conftest.py +++ b/openfe/tests/protocols/conftest.py @@ -148,10 +148,10 @@ def aniline_to_benzoic_mapping(benzene_charges): @pytest.fixture -def benzene_many_solv_system(benzene_modifications): +def benzene_many_solv_system(charged_benzene, charged_benzene_modifications): - rdmol_phenol = benzene_modifications['phenol'].to_rdkit() - rdmol_benzo = benzene_modifications['benzonitrile'].to_rdkit() + rdmol_phenol = charged_benzene_modifications['phenol'].to_rdkit() + rdmol_benzo = charged_benzene_modifications['benzonitrile'].to_rdkit() conf_phenol = rdmol_phenol.GetConformer() conf_benzo = rdmol_benzo.GetConformer() @@ -173,7 +173,7 @@ def benzene_many_solv_system(benzene_modifications): ) return openfe.ChemicalSystem( - {'whatligand': benzene_modifications['benzene'], + {'whatligand': charged_benzene, "foo": phenol, "bar": benzo, "solvent": openfe.SolventComponent()}, @@ -181,10 +181,10 @@ def benzene_many_solv_system(benzene_modifications): @pytest.fixture -def toluene_many_solv_system(benzene_modifications): +def toluene_many_solv_system(charged_toluene, charged_benzene_modifications): - rdmol_phenol = benzene_modifications['phenol'].to_rdkit() - rdmol_benzo = benzene_modifications['benzonitrile'].to_rdkit() + rdmol_phenol = charged_benzene_modifications['phenol'].to_rdkit() + rdmol_benzo = charged_benzene_modifications['benzonitrile'].to_rdkit() conf_phenol = rdmol_phenol.GetConformer() conf_benzo = rdmol_benzo.GetConformer() @@ -205,7 +205,7 @@ def toluene_many_solv_system(benzene_modifications): rdmol_benzo, name='benzonitrile' ) return openfe.ChemicalSystem( - {'whatligand': benzene_modifications['toluene'], + {'whatligand': charged_toluene, "foo": phenol, "bar": benzo, "solvent": openfe.SolventComponent()},