Skip to content

Conversation

@lilyminium
Copy link
Member

@lilyminium lilyminium commented Dec 27, 2020

Fixes #99
Fixes #45
Related to #105

These are largely notebooks I made earlier during GSOD and updated a little. However, the diffusion map notebook is new.

Links (forthcoming from RTD):

New

Slightly updated

  • PCA (description)

Add to #112

@lilyminium lilyminium mentioned this pull request Dec 29, 2020
48 tasks
@lilyminium lilyminium requested a review from orbeckst January 4, 2021 22:33
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, only managed to look at 3 notebooks so far:

hole2 notebook

  • I would move some of the general HOLE description for the PDB section to the front, then say that you can either run it like the hole binary (PDB) or on a trajectory/Universe, just to make clearer why you'd do one or the other
  • MDAnalysis.analysis.hole.HOLE -> MDAnalysis.analysis.hole2.HOLE
  • **tmp*/pdb_name.pdb**: : not properly formatted

Diffusion map

  • "where the transitions between conformational states is not well-sampled" -> "where the transitions between conformational states are not well-sampled" (is -> are)

Just for the future: maybe we should use the AdK Transitions DIMS Dataset for the DiffusionMap analysis – instead of a single DIMS trajectory? Might make the output clearer.

Or even better, use a trajectory that shows repeated transitions between states – could be a small organic molecule or a fancy folding trajectory.

PCA

  • Reference to a general paper on PCA (like in DiffusionMap), e.g. (pretty much randomly pulled from my master bibtex file):
    • [1] A. Amadei, A. B. Linssen, and H. J. Berendsen, “Essential dynamics of proteins,” Proteins, vol. 17, pp. 412–25, Dec 1993.
    • [2] I. T. Joliffe, Principal component analysis. Springer Series in Statistics, New York: Springer, 2 ed., 2002.
    • [3] F. Sittel, A. Jain, and G. Stock, “Principal component analysis of molecular dynamics: on the use of cartesian vs. internal coordinates,” J Chem Phys, vol. 141, p. 014111, Jul 2014.
    • [4] F. Sittel and G. Stock, “Perspective: Identification of collective variables and metastable states of protein dynamics,” The Journal of Chemical Physics, vol. 149, no. 15, p. 150901, 2018.
  • What is the warning "For best results, your trajectory should be aligned on your atom group selection before you run the analysis. Setting align=True will not give correct results in the PCA." about? If align=True in PCA does not do the right thing then we should remove it, shouldn't we?
  • The projection equation \mathbf{r}_i(t) = \mathbf{w}_i(t) \times \mathbf{u}_i + \mathbf{\overline{r}} uses "x" for the outer product. This seems prone to misinterpretation. Maybe write it out with explicit sums or use a different symbol (not the "cross product").

@lilyminium
Copy link
Member Author

Thanks for the review, @orbeckst! I edited the things you mentioned, although I may leave migrating the DiffusionMap dataset for the future.

What is the warning "For best results, your trajectory should be aligned on your atom group selection before you run the analysis. Setting align=True will not give correct results in the PCA." about? If align=True in PCA does not do the right thing then we should remove it, shouldn't we?

This is from 0.20.1 -- it has been fixed for 1.0.0. I remove this warning in #120.

@orbeckst orbeckst self-requested a review January 5, 2021 15:55
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuation review... still not done (but wasted more time on 3D contour plotting than I'd care to admit...)

ParmEd

Very nice, didn't find anything.

The OpenMM part is a bonus and shows how MDA is useful in analysis.

Density

Very useful example with the PBC.

  • nice to have: embedded image of the view1 and view2 of the system
  • GROMACS trjconv argument in the table comes as a surprise. Add a sentence with a link to the gmx trjconv program before the table to note that these are operations commonly performed with gmx trjconv. Btw, what are the equivalent operations in VMD/pbc util or cpptraj? – @IAlibay ?
  • link to docs for on-the-fly transformations
  • Instead of calculating your own midpoints, use dens.density.midpoints. In general, I'd use the Density object, which is the result of the analysis instead of private attributes of DensityAnalysis
  • I have more suggestions for 3D plotting in https://gist.github.com/orbeckst/a6a61c8f20a434c1c7520cd51c281522 (including nglview density and matplotlib surface contours, mostly based on https://stackoverflow.com/a/35472146

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuation review ... and final notebook:

H-bond analysis

  • An introductory sentence on what is generally considered a hydrogen bond would be nice.
  • add link to API docs (I'd prefer if all mentioning of MDAnalysis objects such as MDAnalysis.analysis.hydrogenbonds.HydrogenBondAnalysis linked to the docs but presumably there's not markup in the notebooks equivalent to the sphinx roles that enable intersphinx linking?)
  • The conditions for guessing were clearly spelled out – better than in the API docs! Is there a way for a user to create a list of all donors/acceptors that were considered for their system, e.g., so that they can check that whatever they think ought to be included is actually included (e.g., Methionine S_delta as acceptor (Gregoret et al, Proteins 9 1991, doi:10.1002/prot.340090204.))?
  • update_selections should be explained and point out that performance can be increased with False if the groups are static.

SUMMARY

Great content, we just need to make sure that users find it!

@lilyminium
Copy link
Member Author

@orbeckst thanks for the reviews and sorry for taking months to get back to you. In the interest of not forgetting about this for more months, I might merge tomorrow morning unless new changes are requested.

@orbeckst
Copy link
Member

orbeckst commented Jun 21, 2021 via email

@lilyminium lilyminium merged commit e88eeef into MDAnalysis:master Jun 21, 2021
@lilyminium lilyminium deleted the new branch June 21, 2021 16:14
lilyminium added a commit that referenced this pull request Jan 21, 2022
lilyminium added a commit that referenced this pull request Jan 22, 2022
lilyminium added a commit that referenced this pull request Jan 22, 2022
lilyminium added a commit that referenced this pull request Jan 22, 2022
lilyminium added a commit that referenced this pull request Jan 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sections to be added Finalise script behaviour

2 participants