-
Notifications
You must be signed in to change notification settings - Fork 22
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
Update topology/system particle bookkeeping in OpenMM #1051
base: develop
Are you sure you want to change the base?
Update topology/system particle bookkeeping in OpenMM #1051
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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 very brief (i.e. had 10 mins to read the code) and probably way too specifically opiniated to OpenFE workfllows views.
@@ -173,6 +186,21 @@ def to_openmm_topology( | |||
order=bond_order, | |||
) | |||
|
|||
if has_virtual_sites and not collate: | |||
virtual_site_chain = openmm_topology.addChain(atom_chain_id) | |||
virtual_site_residue = openmm_topology.addResidue("VS", virtual_site_chain) |
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.
From a practical standpoint, a reference to the original residue would possibly be useful - i.e. when I'm doing "solvent within N of Y" I'm looking for a residue name that matches SOL | WAT | HOH not VS.
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 this would be solved by your suggestion later - that each "virtual site residue" be named according to the molecule/residue it's from?
virtual_site_residue = openmm_topology.addResidue("VS", virtual_site_chain) | ||
|
||
for molecule_index, molecule in enumerate(topology.molecules): | ||
virtual_sites_in_this_molecule: list[VirtualSiteKey] = molecule_virtual_site_map[molecule_index] |
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.
Only briefly looking at things, but it looks to me like that we're really needing is this map. If we could map things to/from the collated form then yes we have to plan for this map being optionally around, but at the same time it's not crazy extra amounts of work to work around it.
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'd like to expose a map like this to the user - I have to re-create it a number of times while exporting, might as well cache it on the Interchange
itself or do something like that - but want to be careful and deliberate when making that change
interchange | ||
The Interchange object to convert to an OpenMM Topology. | ||
collate | ||
If False, the default, all virtual sites will be added to a single residue at the end of the topology. |
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.
single residue
I'm sure I'm missing something important here - is there a practical advantage to doing a single residue rather than a residue per molecule's worth of virtual sites?
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.
A little easier to group everything together but nothing significant. I suppose I could have each molecule's virtual site(s) be given their own residue?
Is there an upper bound on residue numbers for some formats? A large-but-not-impractical number of waters, say 20,000, would create an extra 20,000 residues just for the virtual sites of each
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.
upper bound on residue numbers
I could see AMBER and GROMACS being issues here :/ (at least iirc AMBER is still technically fixed width in parm7).
Description
Resolves #1049
Checklist