From a9a53578ffee453d292044afdc98acee775b00d7 Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Thu, 6 Feb 2025 08:46:27 -0800 Subject: [PATCH 1/5] move check partial charges into class --- gufe/components/explicitmoleculecomponent.py | 122 +++++++++---------- 1 file changed, 59 insertions(+), 63 deletions(-) diff --git a/gufe/components/explicitmoleculecomponent.py b/gufe/components/explicitmoleculecomponent.py index 827c65b8..b101544a 100644 --- a/gufe/components/explicitmoleculecomponent.py +++ b/gufe/components/explicitmoleculecomponent.py @@ -35,68 +35,6 @@ def _ensure_ofe_name(mol: RDKitMol, name: str) -> str: return name -def _check_partial_charges(mol: RDKitMol, logger=None) -> None: - """ - Checks for the presence of partial charges. - - Note - ---- - We ensure the charges are set as atom properties - to ensure they are detected by OpenFF - - Raises - ------ - ValueError - * If the partial charges are not of length atoms. - * If the sum of partial charges is not equal to the - formal charge. - UserWarning - * If partial charges are found. - * If the partial charges are near 0 for all atoms. - """ - if "atom.dprop.PartialCharge" not in mol.GetPropNames(): - return - - p_chgs = np.array(mol.GetProp("atom.dprop.PartialCharge").split(), dtype=float) - - if len(p_chgs) != mol.GetNumAtoms(): - errmsg = f"Incorrect number of partial charges: {len(p_chgs)} " f" were provided for {mol.GetNumAtoms()} atoms" - raise ValueError(errmsg) - - if abs(sum(p_chgs) - Chem.GetFormalCharge(mol)) > 0.01: - errmsg = ( - f"Sum of partial charges {sum(p_chgs)} differs from " f"RDKit formal charge {Chem.GetFormalCharge(mol)}" - ) - raise ValueError(errmsg) - - # set the charges on the atoms if not already set - for i, charge in enumerate(p_chgs): - atom = mol.GetAtomWithIdx(i) - if not atom.HasProp("PartialCharge"): - atom.SetDoubleProp("PartialCharge", charge) - else: - atom_charge = atom.GetDoubleProp("PartialCharge") - if not np.isclose(atom_charge, charge): - errmsg = ( - f"non-equivalent partial charges between atom and " f"molecule properties: {atom_charge} {charge}" - ) - raise ValueError(errmsg) - - if np.all(np.isclose(p_chgs, 0.0)): - wmsg = "Partial charges provided all equal to zero. These may be ignored by some Protocols." - warnings.warn(wmsg) - else: - msg = "Partial charges have been provided" - if name := mol.GetProp("ofe-name"): - msg += f" for {name}" - msg += ", these will preferentially be used instead of generating new partial charges." - - if logger is None: - logger = logging.getLogger(__name__) - - logger.info(msg) - - class ExplicitMoleculeComponent(Component): """Base class for explicit molecules. @@ -110,7 +48,6 @@ class ExplicitMoleculeComponent(Component): def __init__(self, rdkit: RDKitMol, name: str = ""): name = _ensure_ofe_name(rdkit, name) - _check_partial_charges(rdkit, logger=self.logger) conformers = list(rdkit.GetConformers()) if not conformers: raise ValueError("Molecule was provided with no conformers.") @@ -129,6 +66,9 @@ def __init__(self, rdkit: RDKitMol, name: str = ""): self._smiles: Optional[str] = None self._name = name + self._check_partial_charges() + + def __getstate__(self): # TODO: check that RDKit setting is set before issuing warning if Chem.GetDefaultPickleProperties() != Chem.PropertyPickleOptions.AllProps: @@ -186,3 +126,59 @@ def _from_dict(cls, d: dict): def _to_dict(self) -> dict: raise NotImplementedError() + + def _check_partial_charges(self) -> None: + """ + Checks for the presence of partial charges. + + Note + ---- + We ensure the charges are set as atom properties + to ensure they are detected by OpenFF + + Raises + ------ + ValueError + * If the partial charges are not of length atoms. + * If the sum of partial charges is not equal to the + formal charge. + UserWarning + * If partial charges are found. + * If the partial charges are near 0 for all atoms. + """ + mol = self._rdkit + + if "atom.dprop.PartialCharge" not in mol.GetPropNames(): + return + + p_chgs = np.array(mol.GetProp("atom.dprop.PartialCharge").split(), dtype=float) + + if len(p_chgs) != mol.GetNumAtoms(): + errmsg = f"Incorrect number of partial charges: {len(p_chgs)} " f" were provided for {mol.GetNumAtoms()} atoms" + raise ValueError(errmsg) + + if abs(sum(p_chgs) - Chem.GetFormalCharge(mol)) > 0.01: + errmsg = ( + f"Sum of partial charges {sum(p_chgs)} differs from " f"RDKit formal charge {Chem.GetFormalCharge(mol)}" + ) + raise ValueError(errmsg) + + # set the charges on the atoms if not already set + for i, charge in enumerate(p_chgs): + atom = mol.GetAtomWithIdx(i) + if not atom.HasProp("PartialCharge"): + atom.SetDoubleProp("PartialCharge", charge) + else: + atom_charge = atom.GetDoubleProp("PartialCharge") + if not np.isclose(atom_charge, charge): + errmsg = ( + f"non-equivalent partial charges between atom and " f"molecule properties: {atom_charge} {charge}" + ) + raise ValueError(errmsg) + + if np.all(np.isclose(p_chgs, 0.0)): + wmsg = "Partial charges provided all equal to zero. These may be ignored by some Protocols." + warnings.warn(wmsg) + else: + msg = f"Partial charges are present for {self.key} (name: {self.name})" + self.logger.info(msg) \ No newline at end of file From bde2435b307a6d434cd4e45070edf82b7d6a635f Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Thu, 13 Feb 2025 15:43:13 -0800 Subject: [PATCH 2/5] remove unneeded test --- gufe/tests/test_smallmoleculecomponent.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/gufe/tests/test_smallmoleculecomponent.py b/gufe/tests/test_smallmoleculecomponent.py index 9287c79d..a5e00e1e 100644 --- a/gufe/tests/test_smallmoleculecomponent.py +++ b/gufe/tests/test_smallmoleculecomponent.py @@ -22,7 +22,7 @@ import gufe from gufe import SmallMoleculeComponent -from gufe.components.explicitmoleculecomponent import _check_partial_charges, _ensure_ofe_name +from gufe.components.explicitmoleculecomponent import _ensure_ofe_name from gufe.tokenization import TOKENIZABLE_REGISTRY from .test_explicitmoleculecomponent import ExplicitMoleculeComponentMixin @@ -248,12 +248,6 @@ def charged_off_ethane(self, named_ethane): off_ethane.assign_partial_charges(partial_charge_method="am1bcc") return off_ethane - def test_check_partial_charges_without_gufe_logger(self, charged_off_ethane, caplog): - rd_mol = charged_off_ethane.to_rdkit() - caplog.set_level(logging.INFO) - _check_partial_charges(rd_mol, logger=None) - assert "Partial charges have been provided for ethane, these" in caplog.text - def test_partial_charges_logging(self, charged_off_ethane, caplog): caplog.set_level(logging.INFO) SmallMoleculeComponent.from_openff(charged_off_ethane) From db149357cc08425f55584c65a5af5c4400dcbeed Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Thu, 13 Feb 2025 16:11:56 -0800 Subject: [PATCH 3/5] updating tests to match correct string, also adding quotes for name in output --- gufe/components/explicitmoleculecomponent.py | 2 +- gufe/tests/test_smallmoleculecomponent.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gufe/components/explicitmoleculecomponent.py b/gufe/components/explicitmoleculecomponent.py index b101544a..9e8e8cfc 100644 --- a/gufe/components/explicitmoleculecomponent.py +++ b/gufe/components/explicitmoleculecomponent.py @@ -180,5 +180,5 @@ def _check_partial_charges(self) -> None: wmsg = "Partial charges provided all equal to zero. These may be ignored by some Protocols." warnings.warn(wmsg) else: - msg = f"Partial charges are present for {self.key} (name: {self.name})" + msg = f"Partial charges are present for {self.key} (name: '{self.name}')" self.logger.info(msg) \ No newline at end of file diff --git a/gufe/tests/test_smallmoleculecomponent.py b/gufe/tests/test_smallmoleculecomponent.py index a5e00e1e..aab901eb 100644 --- a/gufe/tests/test_smallmoleculecomponent.py +++ b/gufe/tests/test_smallmoleculecomponent.py @@ -252,7 +252,7 @@ def test_partial_charges_logging(self, charged_off_ethane, caplog): caplog.set_level(logging.INFO) SmallMoleculeComponent.from_openff(charged_off_ethane) - assert "Partial charges have been provided" in caplog.text + assert "Partial charges are present for SmallMoleculeComponent-" in caplog.text def test_partial_charges_zero_warning(self, charged_off_ethane): charged_off_ethane.partial_charges[:] = 0 * unit.elementary_charge @@ -286,7 +286,7 @@ def test_partial_charges_applied_to_atoms(self, caplog): caplog.set_level(logging.INFO) ofe = SmallMoleculeComponent.from_rdkit(mol) - assert "Partial charges have been provided" in caplog.text + assert "Partial charges are present for SmallMoleculeComponent-" in caplog.text # convert to openff and make sure the charges are set off_mol = ofe.to_openff() From 34408657fc776bd7142c2ee106a67c3dda7cc73d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 14 Feb 2025 00:14:57 +0000 Subject: [PATCH 4/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- gufe/components/explicitmoleculecomponent.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/gufe/components/explicitmoleculecomponent.py b/gufe/components/explicitmoleculecomponent.py index 9e8e8cfc..66f3177e 100644 --- a/gufe/components/explicitmoleculecomponent.py +++ b/gufe/components/explicitmoleculecomponent.py @@ -68,7 +68,6 @@ def __init__(self, rdkit: RDKitMol, name: str = ""): self._check_partial_charges() - def __getstate__(self): # TODO: check that RDKit setting is set before issuing warning if Chem.GetDefaultPickleProperties() != Chem.PropertyPickleOptions.AllProps: @@ -154,7 +153,9 @@ def _check_partial_charges(self) -> None: p_chgs = np.array(mol.GetProp("atom.dprop.PartialCharge").split(), dtype=float) if len(p_chgs) != mol.GetNumAtoms(): - errmsg = f"Incorrect number of partial charges: {len(p_chgs)} " f" were provided for {mol.GetNumAtoms()} atoms" + errmsg = ( + f"Incorrect number of partial charges: {len(p_chgs)} " f" were provided for {mol.GetNumAtoms()} atoms" + ) raise ValueError(errmsg) if abs(sum(p_chgs) - Chem.GetFormalCharge(mol)) > 0.01: @@ -172,7 +173,8 @@ def _check_partial_charges(self) -> None: atom_charge = atom.GetDoubleProp("PartialCharge") if not np.isclose(atom_charge, charge): errmsg = ( - f"non-equivalent partial charges between atom and " f"molecule properties: {atom_charge} {charge}" + f"non-equivalent partial charges between atom and " + f"molecule properties: {atom_charge} {charge}" ) raise ValueError(errmsg) @@ -181,4 +183,4 @@ def _check_partial_charges(self) -> None: warnings.warn(wmsg) else: msg = f"Partial charges are present for {self.key} (name: '{self.name}')" - self.logger.info(msg) \ No newline at end of file + self.logger.info(msg) From 41ab5eb72ea31ea280b645d371d10b67d313f378 Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Thu, 13 Feb 2025 16:22:33 -0800 Subject: [PATCH 5/5] add news item --- news/partial_charge_warning_to_logger.rst | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 news/partial_charge_warning_to_logger.rst diff --git a/news/partial_charge_warning_to_logger.rst b/news/partial_charge_warning_to_logger.rst new file mode 100644 index 00000000..deb15d61 --- /dev/null +++ b/news/partial_charge_warning_to_logger.rst @@ -0,0 +1,23 @@ +**Added:** + +* + +**Changed:** + +* The message stating that partial charges are already present in an ``ExplicitMoleculeComponent`` is now included in ``logger.info``, rather than as a warning message. This should make output significantly less noisy for some users. + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* + +**Security:** + +*