added stubs for Molecule conversions #58

richardjgowers · 2022-02-16T11:42:40Z

added Molecule conversion methods

codecov · 2022-02-16T11:45:11Z

Codecov Report

Merging #58 (d047c94) into main (6c888af) will increase coverage by 0.24%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   98.30%   98.55%   +0.24%     
==========================================
  Files          25       37      +12     
  Lines         473      691     +218     
==========================================
+ Hits          465      681     +216     
- Misses          8       10       +2

Impacted Files	Coverage Δ
openfe/setup/__init__.py	`100.00% <ø> (ø)`
openfe/tests/setup/test_molecule.py	`96.61% <94.11%> (-3.39%)`	⬇️
openfe/setup/atommapper.py	`100.00% <100.00%> (ø)`
openfe/setup/atommapping.py	`100.00% <100.00%> (ø)`
openfe/setup/lomap_mapper.py	`100.00% <100.00%> (ø)`
openfe/setup/molecule.py	`100.00% <100.00%> (ø)`
openfe/utils/__init__.py	`100.00% <100.00%> (ø)`
openfe/utils/typing.py	`100.00% <100.00%> (ø)`
openfecli/parameters/__init__.py	`100.00% <0.00%> (ø)`
openfecli/tests/parameters/test_utils.py	`100.00% <0.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c888af...d047c94. Read the comment docs.

dwhswenson

I'm a little unsure about implicitly treating all arguments with this, although I suppose a user who really needs both our Molecule and rdkit.Mol can just avoid using the decorator (mol.rdkit is not a heavy piece of boilerplate).

A couple alternatives that might be worth considering:

Require argument names in the decorator, so you decorate as @as_rdkit('mol1', 'mol2'). For implementation, inspect.Signature.bind can do most of the heavy lifting to correctly associate argument name with value passed by user, whether in args or kwargs.
If we only need this on AtomMappers, then it might be more straightforward to subclass as RDKitAtomMapper, etc. (But I suspect may be are other places where this will be useful, even if my tired brain can't think of them this morning.)

Not that either of those are necessarily better, but they're worth consideration.

I'm definitely +1 on the Molecule.oechem, Molecule.openff properties.

Handling kwargs in the decorators is the only thing I see as a required change. We can't know whether the user is passing the molecules as args or kwargs.

dwhswenson · 2022-02-16T15:03:22Z

openfe/setup/molecule.py

+def as_rdkit(func):
+    @wraps(func)
+    def inner(*args, **kwargs):
+        args = (a.rdkit if isinstance(a, Molecule) else a for a in args)


definitely need to treat kwargs as well (in this and other decorator)

richardjgowers · 2022-02-16T16:25:29Z

Maybe a subclass for a particular toolkit is a more intuitive way for people to write these too…

…

On Wed, Feb 16, 2022 at 15:14, David W.H. Swenson ***@***.***> wrote: ***@***.**** requested changes on this pull request. I'm a little unsure about implicitly treating all arguments with this, although I suppose a user who really needs both our Molecule and rdkit.Mol can just avoid using the decorator (mol.rdkit is not a heavy piece of boilerplate). A couple alternatives that might be worth considering: 1. Require argument names in the decorator, so you decorate as @as_rdkit('mol1', 'mol2'). For implementation, inspect.Signature.bind <https://docs.python.org/3/library/inspect.html#inspect.Signature.bind> can do most of the heavy lifting to correctly associate argument name with value passed by user, whether in args or kwargs. 2. If we only need this on AtomMappers, then it might be more straightforward to subclass as RDKitAtomMapper, etc. (But I suspect may be are other places where this will be useful, even if my tired brain can't think of them this morning.) Not that either of those are necessarily better, but they're worth consideration. I'm definitely +1 on the Molecule.oechem, Molecule.openff properties. Handling kwargs in the decorators is the only thing I see as a required change. We can't know whether the user is passing the molecules as args or kwargs. ------------------------------ In openfe/setup/molecule.py <#58 (comment)>: > @@ -42,3 +55,21 @@ def __hash__(self): def __eq__(self, other): return hash(self) == hash(other) + + +def as_rdkit(func): + @wraps(func) + def inner(*args, **kwargs): + args = (a.rdkit if isinstance(a, Molecule) else a for a in args) definitely need to treat kwargs as well (in this and other decorator) — Reply to this email directly, view it on GitHub <#58 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSGB4QMMMHYFQAW2FPRTLU3O5LZANCNFSM5ORLJ3RQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

IAlibay · 2022-02-17T17:43:30Z

environment.yml

@@ -10,6 +10,7 @@ dependencies:
  - pytest-xdist
  - pytest-cov
  - coverage
+  - openff-toolkit


add jaimergp/label/unsupported-cudatoolkit-shim to channels above and channel-priority: strict to the conda-incubator/setup-miniconda@v2 in the ci.yaml file?

Nah I think we're just sucking it up and installing cuda

are we treating environment.yml as something users will use?

I would think perhaps code contributors, but not users -- env for users is defined by the recipe in the feedstock

In that case I'm not sure why we would needlessly slowdown CI by not including the shim?

proposed Molecule converter decorators

fixup test

Ambertools not available for windows

pep8speaks · 2022-02-21T16:11:49Z

Hello @richardjgowers! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-02-24 16:17:42 UTC

openfe/tests/setup/test_molecule.py

dwhswenson

Main issue is that I don't think its good to drop Windows support entirely. At least, I will entirely rely on CI to tell me that code still runs on Win, and I thought that was an important target for our users, especially in the setup stage.

Otherwise, please change the title -- these are no longer stubs; it's the full molecule conversion! (At least, in the "to" direction.) 😊

.github/workflows/ci.yaml

openfe/setup/molecule.py

richardjgowers · 2022-02-22T21:57:28Z

Yeah Windows was dropped because Ambertools didn’t have a package, so it won’t be a fix done before Tuesday.

…

On Tue, Feb 22, 2022 at 21:54, Irfan Alibay ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .github/workflows/ci.yaml <#58 (comment)>: > @@ -25,11 +25,14 @@ jobs: strategy: fail-fast: false matrix: - os: ['ubuntu', 'windows', 'macos'] + os: ['ubuntu', 'macos'] Main issue is that I don't think its good to drop Windows support entirely. Yikes didn't see that change on first reivew. I agree with you here @dwhswenson <https://github.com/dwhswenson>. Plus I think that Windows is pretty important for industrial users too, but it seems like OFF has just decided not bother: https://open-forcefield-toolkit.readthedocs.io/en/latest/installation.html#os-support Maybe in the first instance we drop support, and check if it's a deal breaker on Tuesday? Mainly saying this as "we probably want this in before Tuesday, and dealing with whatever breaks in CI before the release is going to be more time than we probably can afford it". — Reply to this email directly, view it on GitHub <#58 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSGB3G7NMPEAJTYV4TIXLU4QA3BANCNFSM5ORLJ3RQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

richardjgowers · 2022-02-23T19:24:17Z

@dwhswenson what do you think about renaming the .rdkit etc to to_rdkit() to a) match OFF and b) make it clearer that a copy is happening?

dwhswenson · 2022-02-23T19:39:44Z

@dwhswenson what do you think about renaming the .rdkit etc to to_rdkit() to a) match OFF and b) make it clearer that a copy is happening?

Mixed feelings. I see the value in matching OFF. Do we actually intend this to be a copy, though? I thought we were going to cache it. (I did the copy on the input via from_rdkit).

That said, if we make it a copy on output, then I think making it a method, not property, is required. Otherwise you get mol.rdkit is not mol.rdkit.

richardjgowers · 2022-02-23T22:28:15Z

Yeah so the deliberate copy means we don’t have to track what changes are happening to the object downstream, so accessing .rdkit then modifying it shouldn’t modify the originating Molecule. This then means we aren’t promising which object is our internal representation

…

On Wed, Feb 23, 2022 at 19:39, David W.H. Swenson ***@***.***> wrote: @dwhswenson <https://github.com/dwhswenson> what do you think about renaming the .rdkit etc to to_rdkit() to a) match OFF and b) make it clearer that a copy is happening? Mixed feelings. I see the value in matching OFF. Do we actually intend this to be a copy, though? I thought we were going to cache it. (I did the copy on the input via from_rdkit). That said, if we make it a copy on output, then I think making it a method, not property, is required. Otherwise you get mol.rdkit is not mol.rdkit. — Reply to this email directly, view it on GitHub <#58 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSGBYB2U24D43ZAYCLKE3U4UZYXANCNFSM5ORLJ3RQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dwhswenson · 2022-02-23T22:33:57Z

Yeah, these are good points on why returning a copy is a good idea. In that case, I'm strongly in favor of to_rdkit(). I think that if we return a copy, it should be mol.to_rdkit(); if we cache the object once created and always return the cached, it should be mol.rdkit.

richardjgowers · 2022-02-24T17:30:58Z

openfe/setup/molecule.py

    def __init__(self, rdkit: RDKitMol, name: str = ""):
-        self._rdkit = rdkit
+        # internally we store as RDKit, for now...
+        self._rdkit = Chem.Mol(rdkit)
        self._hash = hashmol(self._rdkit, name=name)

-    # property for immutability; also may allow in-class type conversion
-    @property
-    def rdkit(self) -> RDKitMol:
-        """RDKit representation of this molecule"""
-        return self._rdkit
+    def to_rdkit(self) -> RDKitMol:
+        """Return an RDKit copied representation of this molecule"""
+        return Chem.Mol(self._rdkit)
+
+    @classmethod
+    def from_rdkit(cls, rdkit: RDKitMol, name: str = ""):
+        """Create a Molecule copying the input from an rdkit Mol"""
+        return cls(rdkit=Chem.Mol(rdkit), name=name)
+
+    def to_openeye(self) -> OEMol:
+        """OEChem representation of this molecule"""
+        return self.to_openff().to_openeye()
+
+    @classmethod
+    def from_openeye(cls, oemol: OEMol, name: str = ""):
+        raise NotImplementedError
+
+    def to_openff(self) -> OFFMolecule:
+        """OpenFF Toolkit representation of this molecule"""
+        m = OFFMolecule(self._rdkit, allow_undefined_stereo=True)
+        m.name = self.name
+
+        return m
+
+    @classmethod
+    def from_openff(cls, openff: OFFMolecule, name: str = ""):
+        raise NotImplementedError


@dwhswenson so what I've done here is everything is a copy. So on creation, we copy the rdkit Mol, and all to_X are copies. The Molecule is then therefore read-only (and documented as such). Handling manipulation of a Molecule is out of scope for us (i.e. if you want to change a Molecule, recreate it).

100% agree with the approach. Reviewing details now.

dwhswenson

2 minor suggestions; otherwise LGTM. Please make those changes unless they don't make sense, but marking this approved; I don't think I need to review after changes.

dwhswenson · 2022-02-24T17:36:30Z

openfe/setup/molecule.py

+    @classmethod
+    def from_rdkit(cls, rdkit: RDKitMol, name: str = ""):
+        """Create a Molecule copying the input from an rdkit Mol"""
+        return cls(rdkit=Chem.Mol(rdkit), name=name)


Suggested change

return cls(rdkit=Chem.Mol(rdkit), name=name)

return cls(rdkit=rdkit, name=name)

since we copy in the __init__, no need to double the copying here.

dwhswenson · 2022-02-24T17:37:42Z

openfe/tests/setup/test_molecule.py

 import pytest

+try:
+    from openeye import oechem
+except ImportError:


Suggested change

except ImportError:

except ImportError: # -no-cov-

looks like this line codecov is complaining about this; maybe we add something for the test matrix to test against a minimal set of reqs? For now, ignore coverage here.

richardjgowers requested review from dwhswenson, mikemhenry and IAlibay February 16, 2022 11:42

dwhswenson requested changes Feb 16, 2022

View reviewed changes

IAlibay reviewed Feb 17, 2022

View reviewed changes

richardjgowers and others added 7 commits February 21, 2022 12:04

added stubs for Molecule conversions

9409e0b

proposed Molecule converter decorators

roll back as_toolkit decorators for now

1f3dab9

add offtk as dep

fd845ff

add offtk as dep

b607631

added oe license to ci

8c7520f

Update ci.yaml

9f64761

suppress openff tk warnings

f7bce0c

fixup test

richardjgowers force-pushed the Molecule_converts branch from e9c720d to f7bce0c Compare February 21, 2022 12:05

richardjgowers added 4 commits February 21, 2022 15:24

Update ci.yaml

0446e79

Ambertools not available for windows

add oechem conversion test

91ce61f

add oechem to deps

3563f9f

centralise types

859e441

richardjgowers requested a review from dwhswenson February 21, 2022 16:16

richardjgowers commented Feb 22, 2022

View reviewed changes

openfe/tests/setup/test_molecule.py Show resolved Hide resolved

richardjgowers added 2 commits February 22, 2022 17:04

Update ci.yaml

69dbfd8

tidying

eadf188

dwhswenson reviewed Feb 22, 2022

View reviewed changes

.github/workflows/ci.yaml Show resolved Hide resolved

openfe/setup/molecule.py Show resolved Hide resolved

richardjgowers mentioned this pull request Feb 23, 2022

Molecule serialization/deserialization #63

Merged

4 tasks

add from_rdkit

c0829bf

change Molecule convert properties to methods

d047c94

richardjgowers requested a review from dwhswenson February 24, 2022 17:28

richardjgowers commented Feb 24, 2022

View reviewed changes

dwhswenson approved these changes Feb 24, 2022

View reviewed changes

dwhswenson mentioned this pull request Feb 24, 2022

Add visualization to AtomMapping #60

Merged

richardjgowers merged commit 3579a01 into main Feb 24, 2022

richardjgowers deleted the Molecule_converts branch February 24, 2022 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added stubs for Molecule conversions #58

added stubs for Molecule conversions #58

richardjgowers commented Feb 16, 2022 •

edited

Loading

codecov bot commented Feb 16, 2022 •

edited

Loading

dwhswenson left a comment

dwhswenson Feb 16, 2022

richardjgowers commented Feb 16, 2022 via email

IAlibay Feb 17, 2022

richardjgowers Feb 21, 2022

IAlibay Feb 22, 2022

dwhswenson Feb 22, 2022

IAlibay Feb 22, 2022

pep8speaks commented Feb 21, 2022 •

edited

Loading

dwhswenson left a comment

richardjgowers commented Feb 22, 2022 via email

richardjgowers commented Feb 23, 2022

dwhswenson commented Feb 23, 2022

richardjgowers commented Feb 23, 2022 via email

dwhswenson commented Feb 23, 2022

richardjgowers Feb 24, 2022

dwhswenson Feb 24, 2022

dwhswenson left a comment

dwhswenson Feb 24, 2022

dwhswenson Feb 24, 2022

	return cls(rdkit=Chem.Mol(rdkit), name=name)
	return cls(rdkit=rdkit, name=name)

added stubs for Molecule conversions #58

added stubs for Molecule conversions #58

Conversation

richardjgowers commented Feb 16, 2022 • edited Loading

codecov bot commented Feb 16, 2022 • edited Loading

Codecov Report

dwhswenson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardjgowers commented Feb 16, 2022 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Feb 21, 2022 • edited Loading

Comment last updated at 2022-02-24 16:17:42 UTC

dwhswenson left a comment

Choose a reason for hiding this comment

richardjgowers commented Feb 22, 2022 via email

richardjgowers commented Feb 23, 2022

dwhswenson commented Feb 23, 2022

richardjgowers commented Feb 23, 2022 via email

dwhswenson commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwhswenson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardjgowers commented Feb 16, 2022 •

edited

Loading

codecov bot commented Feb 16, 2022 •

edited

Loading

pep8speaks commented Feb 21, 2022 •

edited

Loading