-
Notifications
You must be signed in to change notification settings - Fork 33
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
Abstract_Site_Representations #523
base: old_main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #523 +/- ##
==========================================
- Coverage 90.79% 89.49% -1.30%
==========================================
Files 56 57 +1
Lines 4626 4693 +67
==========================================
Hits 4200 4200
- Misses 426 493 +67
Continue to review full report at Codecov.
|
This pull request introduces 3 alerts when merging 7764272 into fee3df2 - view on LGTM.com new alerts:
|
@chrisjonesBSU @jennyfothergill deferring this to you guys' expertise. |
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.
Forgive me if this review is not helpful--I'm not familiar with gmso.
These ideas are really interesting to me:
Add a Virtual_Site class that is drawn from a grouping of Bead or Atom classes.
Implement relevant usages for information drawn from Virtual_Site.
Add tests for implementation of a topology that includes a virtual site.
I like the idea of using theBead
class to store a coarse grain mapping. (e.g., a thiophene moiety, smiles 'c1cscc1', is mapped to a single bead and maybe theBead
class could store that mapping.)
As for what properties I would use of a bead, I can think of type, position, mass, charge...
gmso/core/bead.py
Outdated
|
||
bead_type_: Optional[AtomType] = Field( | ||
None, | ||
description='AtomType associated with the atom' |
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.
description='AtomType associated with the atom' | |
description='AtomType associated with the bead' |
should this be bead?
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, good catch @jennyfothergill. I'm going to go through and try to write some more implementation for the bead class. Based on the properties you mentioned above, looks like it will act relatively similarly to the current site
class.
I would request that the residues be a part of the atom/bead in all cases, preferably as an attribute. I outline some of the major reasons below. For all GOMC and many other bio-type simulations the residues are required to conduct a standard simulation and analysis. For GOMC the residue names are required for the following reasons:
Other bio simulations in MoSDeFs future:
Specific coarse grain FF example:
|
I wonder if residue/virtual site could be grouped into one attribute. It seems to me that they would both represent a grouping of particles and could contain which particles the residue/virtual site contains, a type name, and an id/index? Is there anything further that a bio reside would need? |
As long as GOMC or bio engines can access this residue for building the engines input files (psf, pdb, and FF files) info we "should" be OK. For example, the residue name will need to be tied to each atom number/ID. There could be some bonding between the same type of residues in a protein, or if side group is added. Therefore, all the residues may not be exactly the same molecularly speaking. Example: A residue at the beginning or end of a protein will be "capped" and have a slightly different structure that the same residue in the middle of the protein. This is the same concept as building a polymer from monomers with the ends being different, except the monomer is the amino acids in the bio case. I just think we should make the residue information flexible as described above so bio application are easily accommodated. Eventually, it would be nice if we can eventually use the crystallography protein data (pdb), or at least a modified mol2 version to input the protein data and keeping all the original information, mol ID, residue names... However, I believe this is a good deal of work. |
I agree with these reasons. I think particular for integration with commonly used bio simulation tools, such as Chimera or Avogadro that rely on residues for most of their tools and analysis, I think this makes sense to implement within this PR.
I see what you're saying @jennyfothergill, but as far as I can tell there might be differences that are worth implementing them separately. I think it makes sense to include in each As far as actually creating this |
for more information, see https://pre-commit.ci
This pull request introduces 3 alerts when merging 9943f31 into 3a387c7 - view on LGTM.com new alerts:
|
The goal of of this PR is to allow support of different site classes beyond the currently implemented
Atom
Class.This PR is linked to Issue #495, and further discussion can be found at this link.
Checklist:
Atom
class to it refers to atom specific behavior, and allows for the addition of site variants.Bead
class that behaves similarly toAtom
class.Bead
site will have different behavior from anAtom
site.Virtual_Site
class that is drawn from a grouping ofBead
orAtom
classes.Virtual_Site
.Topology
.If anyone can link me to some specific use cases, that would be extremely helpful as I start to get develop some of these classes. As discussion for this PR builds, I'll also include a summary of what I think are relevant questions to address and possible solutions.
List of Questions: