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! |
|
@hannahbaumann thanks for cleaning this up its looking great with the new openfe-analysis work! Do you want me to remove the old rendered figures? |
|
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). View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:22Z Line #2. import mdtraj as md Are we using this import anywhere anymore? hannahbaumann commented on 2026-06-10T09:31:46Z Removed this |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:23Z Line #2. tempfactor = 0.25 This is set but never used. hannahbaumann commented on 2026-06-10T09:32:29Z True, removed this! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:23Z Line #4. state_a = sum([u_0.atoms[u_0.atoms.tempfactors == i] for i in (0.0, 0.5, 0.25)]) Could you not do hannahbaumann commented on 2026-06-10T09:32:42Z Thanks, changed this! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:24Z Line #5. w.write(u_0.atoms[state_a.atoms.ix]) Can you not just do hannahbaumann commented on 2026-06-10T09:32:53Z Changed this! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:25Z Line #7. w.write(u_1.atoms[state_b.atoms.ix]) As above, you probably can just write the state_b atomgroup. hannahbaumann commented on 2026-06-10T09:33:01Z Changed this! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:26Z Line #6. septop_lig_A_indices = unk_residues[0].atoms.indices.tolist() [nit] To aid reader comprehension, adding a comment above this line re-iterating that you're getting the two ligands from distinct residues, might help. hannahbaumann commented on 2026-06-10T09:33:16Z Added a comment! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:27Z Should this be hannahbaumann commented on 2026-06-10T09:33:30Z Yes, changed this! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:28Z Line #2. state_a = sum([u_septop_0.select_atoms("protein"), lig_A_0])
It would be cleaner for users to do: hannahbaumann commented on 2026-06-10T09:33:40Z Changed this! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:28Z Line #5. w.write(u_septop_0.atoms[state_a.atoms.ix]) should be able to use state_a directly here hannahbaumann commented on 2026-06-10T09:33:49Z Changed this! |
|
View / edit / reply to this conversation on ReviewNB IAlibay commented on 2026-06-09T13:40:29Z Line #7. w.write(u_septop_1.atoms[state_b.atoms.ix]) As above hannahbaumann commented on 2026-06-10T09:33:59Z Changed this! |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
This is looking good but there's a few things that are worth addressing before merge.
|
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! |
4 participants
Fixes #223