Add BoreschRestraintAnalysis class#123
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
==========================================
+ Coverage 98.26% 98.35% +0.09%
==========================================
Files 9 10 +1
Lines 461 487 +26
==========================================
+ Hits 453 479 +26
Misses 8 8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if len(atomgroup) != 6: | ||
| raise ValueError( | ||
| f"atomgroup must contain exactly 6 atoms (3 host + 3 guest), got {len(atomgroup)}." | ||
| ) |
There was a problem hiding this comment.
Does it make sense to check that there are no bonds between the H and G atoms or that they are in different residues?
There was a problem hiding this comment.
[personal take] I could go either way, but I personally wouldn't bother (i.e. flexibility is always good).
| @@ -0,0 +1,393 @@ | |||
| { | |||
There was a problem hiding this comment.
This could also be used in ABFEs as well, can we make the intro more generic to absolute calculations and then narrow the scope to septop for this example?
Reply via ReviewNB
| @@ -0,0 +1,393 @@ | |||
| { | |||
There was a problem hiding this comment.
typo: includea -> includes
Analysis unit -> Analysis Unit ?
match on - > match outputs["repeat_id"]
Reply via ReviewNB
| @@ -0,0 +1,393 @@ | |||
| { | |||
There was a problem hiding this comment.
Is this only plotting the first and last frame? It makes the plots look strange and quite different from what we would expect in practice, so it might be best to plot more frames to avoid confusing users when they run this analysis. I think we should highlight what the interacting lambda is in the text for each case, as it swaps going from A to B.
Reply via ReviewNB
There was a problem hiding this comment.
This was from a very short simulation, I didn't have one at hand with more frames, will update that after running some more simulations!
| @@ -0,0 +1,393 @@ | |||
| { | |||
There was a problem hiding this comment.
For the future, maybe we should be adding some shaded region for the std of the reference value to indicate what a reasonable fluctuation is.
Reply via ReviewNB
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Only just reviewing the analysis class itself, it looks great! So I'll approve on my end.
That being said, I agree with Josh's comments on the notebook (didn't read too deeply into the notebook details though).
| Notes | ||
| ----- | ||
| Bond length is reported in Angstrom; angles and dihedrals are reported | ||
| in radians, matching the units returned by MDAnalysis's |
There was a problem hiding this comment.
Something to think about doing (not here) is to make the final results united.
3 participants
No description provided.