-
Notifications
You must be signed in to change notification settings - Fork 24
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
Sang nanoparticle #286
base: master
Are you sure you want to change the base?
Sang nanoparticle #286
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.
This already looks like a very good draft! I've added a bunch of comments in order to reshape the organization such that it fits in better with the rest polyply, eventually.
However, to make faster progress and start testing I recommend the following: Try to create a function gen_np
that essentially generates coordinates and itp files for nanoparticles with a signature similar to gen_coords
and gen_itp
. We worry later on how to integrate that with the rest of the functions. In my last review comment I've drafted what such a function could look like in general.
In addition to creating this function I would recommend two other objectives for now:
-
Let's try to externalize data-files for the core and ligands as well as links into the libraries in form of .ff files. In the review I've made some comments on how to do that.
-
Let's try to split parameter generation and coordinate generation into three processors.
- a) First let's try have one Processor for making the core coordinates. See my comment on line 81ff for what I would propose.
- b) Then let's have a a Processor, which does the ligand attachment on the graph but doesn't generate coordinates (i.e. everything that is related to _convert_to_vermouth_molecule) BUT give it the core coordinates in form of a molecule from the previous step. see comment for more details.
- c) Finally we have processor that generates only the coordinates for the ligand attachment. This is not yet implemented as far as I can see, is that right?
@Shtkddud123 thanks for removing the files. Can you also run a 'git rebase master'. That should take care the files are wiped from the record, while keeping all the changes. |
Hi Fabian.
I've rebased the repo. Please let me know if there are issues!
Thanks
Sang
…On Sat, Jan 14, 2023 at 8:29 PM Fabian Grunewald ***@***.***> wrote:
@Shtkddud123 <https://github.com/Shtkddud123> thanks for removing the
files. Can you also run a 'git rebase master'. That should take care the
files are wiped from the record, while keeping all the changes.
—
Reply to this email directly, view it on GitHub
<#286 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALDTKNMATVTPANOOCE7WNSDWSMEBTANCNFSM6AAAAAASOLHDIM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
@MDSYN2019 awesome! you also need to push the rebase |
Hi Fabian,
I think I did push it? Please let me know if there are issues. My apologies
- not used to rebase as I've always had a warning in my head only to use it
in desparate circumstances!
Thanks
Sang
…On Sun, Jan 15, 2023 at 6:56 PM Fabian Grunewald ***@***.***> wrote:
@MDSYN2019 <https://github.com/MDSYN2019> awesome! you also need to push
the rebase
—
Reply to this email directly, view it on GitHub
<#286 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABED7FBRVBG2LBA2TTEREMDWSRB6ZANCNFSM6AAAAAASOLHDIM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@MDSYN2019 sorry something is still amiss; it seems the build directory is still part of the PR. @pckroon removing the superfluous files and doing a git rebase should have solved the problem no? |
Hi Fabian,
I will have another look today. I will also push whatever progress I've
made regarding the NP cores processor today as well.
Best wishes
Sang
…On Thu, Jan 19, 2023 at 8:54 AM Fabian Grunewald ***@***.***> wrote:
@MDSYN2019 <https://github.com/MDSYN2019> sorry something is still amiss;
it seems the build directory is still part of the PR.
@pckroon <https://github.com/pckroon> removing the superfluous files and
doing a git rebase should have solved the problem no?
—
Reply to this email directly, view it on GitHub
<#286 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABED7FDTUUF54G23B3BDIFTWTD6MVANCNFSM6AAAAAASOLHDIM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Not 100% sure, but I think you need to do |
… morse force field parameters with it
@Shtkddud123 I just saw that the rabase has worked! Nice job. There is, however, one more thing which has to go. You're also pushing the 'build' folder. That is your local installation of polyply but should not part of the PR. Removing it the same way as the other files should to the trick. |
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.
Hi Sang,
I was doing some reviewing in different PRs, so I added some comments here as well. Note I did not do an extensive review though, just some suggestions here and there.
Please note implementing the ApplyLinks usage will require to write ff files with links. If you get to that point let me know, because you'll probably need some pointers.
Good job so far!
Fabian
@@ -0,0 +1,105 @@ | |||
generated by VMD, t= 0.000000 |
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.
this file should live in 'polyply/tests/test_data'
@@ -0,0 +1,147 @@ | |||
generated by VMD, t= 0.000000 |
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.
same as before move this to tests/test_data
; ---------------------------------------------------------------------------- | ||
; Topology for C60 fullerene | ||
; Luca Monticelli, December 2007 | ||
; If you find this useful, please cite: | ||
; Monticelli L., J. Chem. Theory Comput. 2012, 8, 1370-1378 | ||
; ---------------------------------------------------------------------------- | ||
; - Geometry from NMR data | ||
; (Yannoni et al, J. Am. Chem. SOC. 1991, 113, 3190-3192) | ||
; | ||
; - Atom type CFUL: from Girifalco, J. Phys. Chem. (1992),96, 858-861 | ||
; | ||
; - Bond and angle parameters: | ||
; same as for CA-CA (and CA-CA-CA) in OPLS-AA (CA is the aromatic carbon) |
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.
FYI for later: citations can be added to .ff files using the [citations]
directive. It requires there to be a .bib file with the actual citations and the key provided in the directive to be the same as in the bibtex file. Also by convention the .bib file needs to have google-scholar format, which is comma separated entries.
system = np_top.convert_to_vermouth_system() | ||
NanoparticleLigandCoordinates().run_system(system) |
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.
instead of this, you can provide the np_top
to generate templates like so:
# this line is needed in order for it to skip generation of the core coordinates.
full_np_metamol.templates = {"rename of core": core coordinates}
LigandGenerator = GenerateTemplates(topology=np_top, max_opt=10).run_system(np_top)
This will generate all Ligand coordinates, which can be accessed through full_np_metamol.templates
self.meta_mol.molecule = self.np_molecule | ||
return self.meta_mol | ||
|
||
def create_itp(self, write_path): |
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.
this function is not needed. Processors just return molecules. I/O is handled at the level of gen_np
with open(str(write_path), "w") as outfile: | ||
write_molecule_itp(self.np_molecule, outfile) | ||
|
||
def create_gro(self, write_path): |
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.
same as above I/O should be handled on the level of gen_np
for index, index2 in enumerate( | ||
list(core_index_relevant.keys())[: self.ligands_N] | ||
): | ||
interaction = vermouth.molecule.Interaction( | ||
atoms=(index2, attachment_list[index] + attachment_index), | ||
parameters=["1", "0.0033", "50000"], | ||
meta={}, | ||
) |
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 understand that this is just a draft. I'll add a comment anyways ;) If you only add an edge between the ligand and the core nanoparticle using molecule.add_edge
, we can in a later step run ApplyLinks
to get the bonded interactions straight from the force-field files. This will make it easier to adopt them later.
So my suggestion:
np_molecule.add_edge(index2, attachment_list[index] + attachment_index)
Then run ApplyLinks() from polyply.src.apply_links in line 251
for node in self.np_molecule.nodes: | ||
# change resname to moltype | ||
self.np_molecule.nodes[node]["resname"] = "TEST" |
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.
This is dangerous. I would suggest the core to have the same rename but keep the ligand resname. We need to pay some attention though later when running the ApplyLinks
…ork on the AA PCBM example
Apologies for the delay. Have been rather busy lately. Created first version of the nanoparticle generator. Some notes:
I've made a PCBM-fullerene nanoparticle with it that adapts a custom vermouth interaction between the CBM and the phenyl group, but this can be replaced with more accurate values. The nanoparticle at the moment adopts the Monticelli model for C60, and ligparm generator with the OPLS FF.
I have included the nanoparticle_generator.py code in the src folder, as well as the sample itp files made from it, and the topology I made with the files. The minimized structure of the nanoparticle is also included there.
The 'Janus' and 'Striped' options do the same thing as none at the moment - this should be expanded soon
I will be adding a 'generic' core generator with this with the next pull request, to adapt making a generic fibanocci sphere with custom constraining bonds
The idea with gold nanoparticles eventually is to add the core as available libraries of cores, where we can attach different types of ligands onto these spheres.
Generating unit cell type nanoparticles for this module is another thing I will be working on for the next iteration.
Please let me know if you need more information.