-
Notifications
You must be signed in to change notification settings - Fork 17
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
(Do not merge) Reference Smoothcore Implementation. #1380
base: master
Are you sure you want to change the base?
Conversation
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.
Nice! Useful generalization and extension of #415
Makes sense, but a couple aspects are outside my current intuition:
- [stability worry] Do any forces become unstable as the virtual sites become very close to the anchor site?
- This appears fine, but some randomized checks (e.g. running MD on random RHFE/RBFE edges with random values of w_i) would provide additional peace of mind before proceeding to the GPU implementation.
- [possible efficiency enhancements] When the group's w_i's approach 1, the collapsed anchor group will look like a single particle that might make stronger-than usual interactions with environment.
- (consider if the group's |\sum_i q_i| is large..., or, even if |\sum_i q_i|== 0, but there's a large number of atoms in the group, then the collapsed site will appear sort of like a single LJ particle with a large epsilon parameter / a very deep well)
- In both cases, I suspect it may help with efficiency if an additional parameter is exposed, which scales the interaction energy by some factor <= 1,
factor * U_interaction_group(x)
... (Or, this could be emulated by users of this function, by scaling down the LJ epsilons by some factor likemean(epsilons) / sum(epsilons)
, etc.)
In the description of #415 , you mentioned this implementation is related to a suggestion from Gilson in the context of docking -- if you happen to have a specific work in mind, capturing that reference in the docstring or PR description would be helpful.
|
Self-consistency w.r.t. Single Topology code at the end-states achieved on a real system (from hif2a) - GPU implementation tomorrow! |
for (int i = 0; i < COORDS_DIM; i++) { | ||
RealType atom_pos = coords[atom_idx * COORDS_DIM + i]; | ||
RealType anchor_pos = coords[anchor_idx * COORDS_DIM + i]; | ||
coords_out[atom_idx * COORDS_DIM + i] = anchor_pos + rescale * (atom_pos - anchor_pos); |
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: could this be simplified to
coords_out[atom_idx * COORDS_DIM + i] = anchor_pos + rescale * (atom_pos - anchor_pos); | |
coords_out[atom_idx * COORDS_DIM + i] = atom_pos + params[atom_idx * PARAMS_DIM + 3] * (anchor_pos - atom_pos); |
(removing rescale
definition)?
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.
rescale is removed by the compiler (would prefer to make the rescaling explicit).
template class SmoothcoreNonbondedInteractionGroup<double>; | ||
template class SmoothcoreNonbondedInteractionGroup<float>; | ||
|
||
} // namespace timemachine |
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.
Request: briefly describe what changed here relative to nonbonded_interaction_group.cu
? Ideally, could create one commit that copies from nonbonded_interaction_group.cu
and a second commit with the changes.
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.
Best way I can think of is to do a manual diff between the two files. This turned out to be much trickier than I initially expected, so for safety reasons I decided to just make a new file for now (and eventually deprecate the old NonbondedInteractionGroup).
@@ -0,0 +1,103 @@ | |||
#pragma once |
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.
Would it be desirable to also have a smoothcore implementation of NonbondedPairListPrecomputed
for intramolecular nonbonded interactions?
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 in theory - but in practice this is tricky since we need to deal with exclusions (1-2/1-3/1-4)
import itertools | ||
|
||
|
||
def nonbonded_block_rescaled(xi, xj, box, params_i, params_j, beta, cutoff, anchors_i, groups_i): |
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: is this currently a different interface than is exposed by the GPU potential? (don't immediately see the concept of "groups" there)
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.
stale - added a standard API and wrapped inside potentials.py
xi_a = xi[np.repeat(anchors_i, [len(x) for x in groups_i])] | ||
|
||
group_idxs = jnp.array(list(itertools.chain(*groups_i))) | ||
|
||
excluded_idxs = list(set(range(len(xi))).difference(group_idxs.tolist())) |
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: can be optimized to avoid Python loops (OK to leave for later)
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.
stale
Checking the gradients for some of these computations - I don't think so. The exact expression for the gradient computation would only ever scale down the forces, not scale up.
Correct, there's two to-be-tuned parameters describing the strength near the
edit: to elaborate a bit more, it's possible that a super-collapsed site may not be 1-step removable. Here, 1-step removable means when going from
As noted, same effect can be achieved by scaling down eps/qs via parameter interpolation
I need to dig this up - having a tough time finding it right now. |
Updated description. |
cb3261b
to
3d1dfe6
Compare
Rebased onto master. |
99dfbf3
to
41a17e9
Compare
This PR designs and implements a new softcore potential, the
SmoothcoreNonbondedInteractionGroup
, that attempts to overcome two major failings of our existing 4D decoupling regime:Instead,
SmoothcoreNonbondedInteractionGroup
takes in a list ofanchoring_idxs
of sizenum_atoms
, specifying each atom's "anchoring" point. Each atom still retains thew
component in the parameters, whose values are now strictly in the closed interval[0, 1]
(as opposed to the old[0, cutoff]
) and monotone along lambda. Here,w=0
still denotes "fully-interacting"/"fully-extended", andw=1
denotes "fully-non-interacting"/"fully-absorbed". Using theanchor_idx
information in conjunction withw
, a vector is drawn and used for scaling purposes to determine the virtual site location:For example:
above:
O
is the anchoring atom for the dummy hydrogen. A scaled vectorr_OH
is used to position the location of the hydrogen.This approach also makes it much easier to balance the net-charge, the all the charge sites are at full strength (but may be smushed/averaged out). At the end-states,
lambda=0
, andlambda=1
, the resulting energies should be identical to the current SingleTopology end-states. A test is added to verify this behavior.Todos
N_ != NR_ + NC_
in interaction group code.