Add simulation extraction and viewing notebook#226
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Overall looks good, thanks for doing this!
Added a few comments for things that we should address.
Co-authored-by: Alyssa Travitz <31974495+atravitz@users.noreply.github.com>
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-03T13:14:02Z Two questions:
1) Could you mention what Transformations are being applied by the method? Also can you mention that there's a separate one for solvent simulations if they want it.
2) Is there anything we can link to regarding when centering is a good idea and when it's not? I can't remember what Justin Lemkhul usually links to. Essentially what I mean here is that it would be good to say "don't center / align if you need to look at how components diffuse over time, etc..." hannahbaumann commented on 2026-06-08T11:45:15Z Added details on the transformations and added a note that the traj should not be aligned when looking at diffusion behavior. I couldn't find a good reference yet though. |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-03T13:14:03Z "We can fix this, however, using the alignment functions in openfe_analysis"
It would be good to reword this as using a helper method to add a transformation to the MDAnalysis Universe. That way folkls that know MDAnalysis things can easily understand what's going on. For those that don't it would be good to link to the Transformations documentions in MDAnalysis (just so that folks understand what that means). And then in the code block add a comment when you apply the transformation. hannahbaumann commented on 2026-06-08T11:48:52Z Reworded, added a link to the MDA docs and a comment in the code block. jthorton commented on 2026-06-09T09:21:49Z This looks really good. One possible bit of info to add is that the resname of the ligand is always UNK from the protocol? IAlibay commented on 2026-06-09T13:24:07Z I would may avoid putting is as "always" but "usually" - in pontibus the ligands have normal residue names. Longer term we need to change this too (it's been a long term wish of mine to make SmallMoleculeComponent have a resname setter). hannahbaumann commented on 2026-06-10T09:32:16Z Thanks, I added a doc string before defining the ligand selection! |
|
Removed this View entire conversation on ReviewNB |
|
Thanks, I added a doc string before defining the ligand selection! View entire conversation on ReviewNB |
|
True, removed this! View entire conversation on ReviewNB |
|
Thanks, changed this! View entire conversation on ReviewNB |
|
Changed this! View entire conversation on ReviewNB |
|
Changed this! View entire conversation on ReviewNB |
|
Added a comment! View entire conversation on ReviewNB |
|
Yes, changed this! View entire conversation on ReviewNB |
|
Changed this! View entire conversation on ReviewNB |
|
Changed this! View entire conversation on ReviewNB |
|
Changed this! View entire conversation on ReviewNB |
|
@jthorton : I removed the images since they were no longer used! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-11T07:23:08Z [nits]
1) Small style discrepancy - you use the backticks around MDAnalysis in one place and then just use a normal link for openfe-analysis and MDAnalysis elsewhere. If you can't use backticks with links, I would just bold the package names.
2) Maybe use backticks around tempfactor?
hannahbaumann commented on 2026-06-11T09:50:39Z Changes this! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-11T07:23:09Z Line #7. ! tar -xzf septop_structural_results.zip Should this be hannahbaumann commented on 2026-06-11T09:50:51Z Using unzip now |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-11T07:23:10Z Line #3. v.render_image(trim=False, factor=4) [nit] Other places you set trim to True, any reason for using False here? hannahbaumann commented on 2026-06-11T09:51:29Z Good catch, I think this was left from playing around with different options, using True here now as well. |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-11T07:23:11Z Line #2. state_a = sum(u_0.atoms[u_0.atoms.tempfactors <= 0.5]) You don't need to do the sum here - the u_0.atoms indexing will create the atomgroup. hannahbaumann commented on 2026-06-11T09:51:46Z Changed this! |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Couple of small things but lgtm! I'll approve early.
jthorton
left a comment
jthorton
left a comment
There was a problem hiding this comment.
Thanks for finishing this up @hannahbaumann LGTM!
|
Changes this! View entire conversation on ReviewNB |
|
Using unzip now View entire conversation on ReviewNB |
|
Good catch, I think this was left from playing around with different options, using True here now as well. View entire conversation on ReviewNB |
|
Changed this! View entire conversation on ReviewNB |
4 participants
Fixes #223