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

Move mapper/mapping (LigandAtomMapping and LigandAtomMapper) implementations to gufe? #224

Closed
IAlibay opened this issue Nov 10, 2022 · 6 comments

Comments

@IAlibay
Copy link
Contributor

IAlibay commented Nov 10, 2022

Recent conversations raised that these implementations may be useful to more than one project.

How best should these classes be shared? Should they be moved to gufe?

@dwhswenson
Copy link
Member

I started drafting an issue about exactly this yesterday. I'm +1 on this change.

It's probably worth a broader discussion on how existing objects should shift between openfe and gufe (e.g., storage).

@richardjgowers
Copy link
Member

Slightly disagree, I think gufe is nice as abstract things. If there's stuff in openfe that is useful to other things I'd rather spin those out (some sort of tools repo for openmm....) and reimport them into openfe.

Ideally version changes in gufe would be rare represent the API changing

@IAlibay
Copy link
Contributor Author

IAlibay commented Nov 10, 2022

One option here could be to make more or less everything that's not a gufe dependency optional in OpenFE? Then folks could just import relevant re-usable bits from openfe without caring too much about dependency size costs?

@IAlibay
Copy link
Contributor Author

IAlibay commented Nov 10, 2022

some sort of tools repo for openmm...

The objects aren't openmm specific right? Could we just create "openfe-tools"? Kinda gets you into namespace packaging though...

@dwhswenson
Copy link
Member

I thought of this yesterday while working on the notebook on writing mappers/scorers/network planners. Note that the mapper I wrote is openfe-specific, even though I'm really trying to teach the gufe API in that notebook. This is because LigandAtomMapper is in openfe, not gufe.

I suppose I could instead teach that by inheriting from gufe.AtomMapper, but I would argue that the better practice is to subclass LigandAtomMapper. I'd could subclass gufe.AtomMapper as a matter of teaching principle, but I'd be demonstrating a bad practice. (And since I know that real developers will just copy-paste whatever I put into that notebook, it matters that the code demonstrates best practices.)

Conceptually, the first mapping thing we teach will be based on mapping SmallMoleculeComponents. That's what I expect people to be familiar with. That's what we use to teach the role of mappings in the larger API. Then we can teach "you can map other components, too." LigandAtomMapper is the specialization of AtomMapper to SmallMoleculeComponents.

I agree that the actual implementations (LomapAtomMapper, etc) belong in openfe; I'm referring here only to the abstract objects.

@richardjgowers
Copy link
Member

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants