-
Notifications
You must be signed in to change notification settings - Fork 3
Protein mutation support in Neq Cycling Setup Unit #106
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
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
IAlibay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of things but otherwise lgtm.
devtools/conda-envs/test_env.yaml
Outdated
| - gufe ~=1.7.1 | ||
| - numpy | ||
| - openfe >=0.15 # TODO: Remove once we don't depend on openfe | ||
| - openfe ~=1.7.0 # TODO: Remove once we don't depend on openfe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're at 1.8 now, but that might be better bumped in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need to change it here since I'm also bumping the python version. Hopefully it would "just work". THanks for the heads up.
| @@ -0,0 +1,49 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the idea of a tutorial, but I would recommend just bumping it to a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already worked on a tutorial, lives in https://github.com/OpenFreeEnergy/feflow/blob/protein-mutation-setup/docs/tutorials/protein-mutations-neq.ipynb
I guess we could consider moving it to the OpenFE Example Notebooks instead in the future.
feflow/protocols/protein_mutation.py
Outdated
|
|
||
| class ProteinMutationProtocol(Protocol): | ||
| pass | ||
| ProteinMutationProtocol = NonEquilibriumCyclingProtocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still todo to remove?
| return topology | ||
|
|
||
|
|
||
| def get_positions_from_component( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can limit it to ExplicitComponents, that way SolventComps don't have it.
feflow/utils/misc.py
Outdated
| ValueError | ||
| If the atom index is not found in the topology. | ||
| """ | ||
| for residue in topology.residues(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely would remove if it's not doing anything.
| solvent_comp_a = ( | ||
| solvent_comps.pop() if solvent_comps else None | ||
| ) # there must be at most one solvent comp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're already volidating it to be a single component, do you need to go thought this? I.e. could you not just make solvents_comp_a == solvent_comps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That's true, I will validate in the _validate method of the protocol, and improve the logic here. The only caveat is that some utility functions expect the component itself, some other expect a list of components (because they are meant to be used for non-solvent components as well), but that shouldn't be an issue. Maybe something we want to change in the future, for all "utils" to have the same typing signature?
| integrator_settings=integrator_settings, | ||
| cache=ffcache, | ||
| has_solvent=solvent_comp is not None, | ||
| has_solvent=bool(solvent_comp_a), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] As far as I know, PEP8 does prefer var is not None over bool(var)
| elif alchemical_settings.softcore_LJ.lower() == "beutler": | ||
| softcore_LJ_v2 = False | ||
| # TODO: We need to test HTF for protein mutation cases, probably. | ||
| # What are ways to quickly check an HTF is correct? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean in terms of testing the hyrid system? Easiest way is to check if the endstates match the energy of the non-alchemical systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good suggestion, I think I'll bump that to a future PR, but sounds like something we could do. Issue raised in #134
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
These changes aim to add support to be able to perform protein mutations in the current
SetupUnitthat theNonEquilibriumCyclingProtocoluses.This mostly implies making sure that the alchemical component is now not assumed to be a
SmallMoleculeComponentbut also aProteinComponent.Utility functions for achieving such purpose were written or modified accordingly. Someof these changes were needed to be made on the
openfeside of things, since we have not yet decided on migrating them tofeflow(this code base).