New Dihedral and Ramachandran Analysis #1997

hfmull · 2018-07-18T18:48:52Z

Changes made in this Pull Request:

added new MDAnalysis.analysis.dihedrals module
use AnalysisBase for new Ramachandran class
new dihedrals_calc function

PR Checklist

Tests?
Docs?
CHANGELOG updated?
[n/a] Issue raised/referenced?

hfmull · 2018-07-18T18:59:25Z

testsuite/MDAnalysisTests/analysis/test_dihedrals.py

+                                  err_msg="error: dihedral angles should "
+                                  "match test values")
+
+    def test_ramachandran_identical_frames(self, universe, tmpdir):


Is it necessary to have this test? It was included because I was learning from the test_rms.py file which had it, but I am not sure of the exact reason to include it and @orbeckst isn't either.

I don't think this is needed here.

kain88-de

thanks for your first contribution. It already looks good. Most of my comments are about style and it would be nice if you address them.

kain88-de · 2018-07-18T19:06:40Z

package/MDAnalysis/analysis/dihedrals.py

+    """
+
+    dihedrals = [atomgroup.dihedral for atomgroup in atomgroups]
+    angles = [dih.value() for dih in dihedrals]


this is a list not a numpy array. I would make this function a one liner

return np.array([ag.dihedral.value() for ag in atomgroups])

kain88-de · 2018-07-18T19:07:11Z

package/MDAnalysis/analysis/dihedrals.py

+            atoms for residues for which phi and psi are calculated
+        start : int, optional
+            starting frame, default None becomes 0.
+        stop : int, optional


aren't we moving those to the run function @orbeckst

yes, you're right, of course – I asked @hfmull to start from analysis.rms.RMSD and forgot that it uses soon-to-be changed call signatures (#1463).

Do we need to keep init start/stop/step for new additions like this one and deprecate them (#1979) right away?? I would say no (don't teach the user something you want them to forget) but I am open to alternative opinions.

still keeping them here makes this behave exactly as any other class. Kind of nice.

It makes more sense to not allow them here if we want them in run

kain88-de · 2018-07-18T19:09:59Z

package/MDAnalysis/analysis/dihedrals.py

+        res_min = np.min(self.atomgroup.universe.select_atoms("protein").residues)
+        res_max = np.max(self.atomgroup.universe.select_atoms("protein").residues)
+        if any([(residue < res_min or residue > res_max) for residue in self.residues]):
+            raise IndexError("Selection exceeds protein length")


how is this possible? Literally how. I would need to get an atom that is not in the universe topology.

I like to check things but I can't think of a case where this would ever be triggered.

if self.atomgroup.residues contains residues which aren't in protein I think?

Solvent molecules are listed as residues in the topology files (at least in the .gro files I've been using) so it possible to have a residue group that contains solvent molecules. The idea was to give more feedback on what the problem actually is rather than being confused when phi_selection() or psi_selection() returned None.

But using ag.issubset(protein) is easier and more robust for the test anyway.

Thanks for the comment. I agree with @orbeckst to use set-semantics because it's easier to understand and more robust. I would also update the warning message to Found atoms outside of protein. Only atoms inside of a 'protein' selection can be used to calculate dihedrals.

kain88-de · 2018-07-18T19:13:47Z

package/MDAnalysis/analysis/dihedrals.py

+        res_max = np.max(self.atomgroup.universe.select_atoms("protein").residues)
+        if any([(residue < res_min or residue > res_max) for residue in self.residues]):
+            raise IndexError("Selection exceeds protein length")
+        elif any([residue == (res_min or res_max) for residue in self.residues]):


I don't think that does what you think it does. I'm sure you want res == res_min or res == res_max for res in residues. The parenthesis should be evaluated first resulting in the test residue == True as the test condition.

I see what you mean. I'll fix that on the next commit.

you can also make use of set operations here

protein = ag.universe.select_atoms('protein') ... elif protein.residues[[0, -1]].issubset(self.residues)

It's more robust like @orbeckst has suggested above. I know the set operations are not well documented but they do make such comparisons easier.

kain88-de · 2018-07-18T19:14:37Z

package/MDAnalysis/analysis/dihedrals.py

+            warnings.warn("Cannot determine phi and psi angles for the first or last residues")
+
+        self.phi_atoms = [residue.phi_selection() for residue in self.residues
+                          if res_min < residue < res_max]


you already checked that this can never happen. You should be fine I don't see the reason for the if here as long as the previous checks really do work. See comment above.

The reason for that conditional is that the function does not allow selections that exceed the protein but it still allows selections that include the first and last residues. However, because it doesn't make sense to calculate the dihedrals for those, they are excluded. The warning just acts as a signal so the user knows that they included those residues in their selection, but that they are not included in the list of angles. This is just to avoid confusion if the number of residues they listed does not match the number of angle sets given.

OK I understand now. you can pre remove them in the selection.

protein = ag.universe.select_atoms('protein') # remove first and last residue if part of selection residues = residues.difference(protein.residues[[0, -1]])

https://www.mdanalysis.org/docs/documentation_pages/core/groups.html
Scroll down to the list of operations.

kain88-de · 2018-07-18T19:18:31Z

package/MDAnalysis/analysis/dihedrals.py

+        if title is not None:
+            ax.set_title(title)
+        for angles in self.angles:
+            ax.plot(angles[0],angles[1], color=color,


a better function would be scatter here. It doesn't need the line plot styling.

ax.scatter(angles[0], angles[1], **kwargs)

You can make this into a one-line and only call into the plotting routine once. Might make a speed difference if you have a lot of frames.

a = self.angles.reshape((np.prod(self.angles.shape[:2]), 2)) ax.scatter(a[0], a[1], **kwargs)

kain88-de · 2018-07-18T19:19:25Z

package/MDAnalysis/analysis/dihedrals.py

+        ax.set_xticks(range(-180,181,60))
+        ax.set_yticks(range(-180,181,60))
+        ax.set_xlabel(r"$\phi$ (deg)")
+        ax.set_ylabel(r"$\psi$ (deg)")


there is a more convenient ax.set(xlabel=..., ylabel=..., xticks=..., yticks=...) function available.

kain88-de · 2018-07-18T19:20:32Z

testsuite/MDAnalysisTests/analysis/test_dihedrals.py

+                                  err_msg="error: dihedral angles should "
+                                  "match test values")
+
+    def test_ramachandran_identical_frames(self, universe, tmpdir):


I don't think this is needed here.

kain88-de · 2018-07-18T19:21:20Z

testsuite/MDAnalysisTests/analysis/test_dihedrals.py

@@ -0,0 +1,57 @@
+import numpy as np
+from numpy.testing import assert_array_almost_equal


please use assert_almost_equal. It does some more equality checks then assert_array_almost_equal

kain88-de · 2018-07-18T19:22:20Z

package/MDAnalysis/analysis/dihedrals.py

+              be added to the current active axes.
+        color : string, optional
+              Color used for markers in the plot; the default color is 'black'.
+        marker : string, optional


use a **kwargs argument to the function. Anything passed to it will automatically be passed to the plot. This gives the greatest freedom for the user.

kain88-de · 2018-07-18T19:26:04Z

Oh and please add this to the documentation generated for the users. @orbeckst can show you how (I assume you are working in his lab)

orbeckst · 2018-07-18T19:59:26Z

@kain88-de @richardjgowers thanks for the comments.

@hfmull is doing a REU in my lab, with the goal to work more on PMDA. We use the dihedral featurization as the initial introduction (and we hopefully get a faster dihedral featurizer out of this MDAnalysis/pmda#45 and perhaps other PMDA analysis blocks).

We just discussed a number of the points that you raised and @hfmull will respond to them as he works through the PR.

richardjgowers · 2018-07-18T20:05:40Z

package/MDAnalysis/analysis/dihedrals.py

+    """
+
+    dihedrals = [atomgroup.dihedral for atomgroup in atomgroups]
+    angles = [dih.value() for dih in dihedrals]


This line isn't wrong, but it would be better to use lib.distances.calc_dihedrals and do them all at once

Oh, that works?

Great!

@hfmull see https://www.mdanalysis.org/mdanalysis/documentation_pages/lib/distances.html#MDAnalysis.lib.distances.calc_dihedrals

@richardjgowers does dih.value() take PBC into account by default?

EDIT: yes. Dihedral.value has pbc=True by default.

lib.distances.calc_dihedrals will want a box from AtomGroup.dimensions so that you get the same results with old and new dihedral calculation.

@hfmull to use calc_dihedrals you will need "vertical atomgroups", ie 4 atomgroups, one representing the first atom in each dihedral, the next the second atom in each dihedral. I'd make these in the __init__ of your class. Then you can call calc_dihedrals(ag1.positions, ag2.positions, ag3.positions, ag4.positions, box=ag1.dimensions)

orbeckst

Looking very good but see my additional comments (in addition to what @kain88-de and @richardjgowers said).

Also

tests for any exceptions/warnings
add yourself to AUTHORS
add stubs to docs (in analysis_modules.rst and new file analysis/dihedrals.rst) and add some text to the top of the dihedrals.py file.

orbeckst · 2018-07-19T00:14:32Z

package/MDAnalysis/analysis/dihedrals.py

+
+from MDAnalysis.analysis.base import AnalysisBase
+
+def dihedral_calc(atomgroups):


The discussion below converged on eliminating this function and using lib.distances.calc_dihedrals.

orbeckst · 2018-07-19T00:15:09Z

testsuite/MDAnalysisTests/datafiles.py

@@ -415,5 +415,8 @@

 GSD = resource_filename(__name__, 'data/example.gsd')

+DihedralsArray = resource_filename(__name__, 'data/adk_oplsaa_dihedrals.npy')


You also need to add the to __all__ (I only learned that from the test that failed!)

orbeckst · 2018-07-19T00:16:46Z

package/MDAnalysis/analysis/dihedrals.py

+    def _conclude(self):
+        self.angles = np.array(self.angles)
+
+    def plot(self, ax=None, color='k', marker='s', title=None):


The method needs a test that checks that it produces an Axes instance. Something like

ax = rama.plot() assert isinstance(ax, plt.matplotlib.Axes)

(or whatever the actual instance is – see type(ax) ... or see other tests, try git grep Axes)

orbeckst · 2018-07-19T00:20:35Z

package/MDAnalysis/analysis/dihedrals.py

+        """
+        if ax is None:
+            ax = plt.gca()
+        ax.axis([-180,180,-180,180])


How do people plot the allowed and marginally allowed regions in "standard" Ramachandran plots? Is there a simple function (something like a spline for a density that gives the correct contours) or a dataset that we could use?

It would be nice if we had a helper function to plot them; this method could then use the helper to plot these regions in the background but anyone else could use it, too.

(Not a requirement for this PR but something to think about.)

I found a data set from this website that was taken from this paper that I was able to make a decent reference graph of. I just don't know where I should put the data need to graph it and where I should cite it. This is just an example of what it looks like with the reference.

orbeckst

looking very good, I only found one small thing and you still have to add the files to the docs.

orbeckst · 2018-07-20T01:13:55Z

package/MDAnalysis/analysis/dihedrals.py

@@ -0,0 +1,105 @@
+import numpy as np


Please add the standard header – copy from the top of rms.py

Please add from __future__ import absolute_import as the very first import with any import from six.moves directly below it. Also include the range import from six.moves

kain88-de · 2018-07-20T07:57:19Z

package/MDAnalysis/analysis/dihedrals.py

+        protein = self.atomgroup.universe.select_atoms("protein").residues
+
+        if not residues.issubset(protein):
+            raise IndexError("Found atoms outside of protein. Only atoms "


I would use a value error here. The IndexError is when you use the square bracket operator.

kain88-de

The changes look good. Just some very minor details left.

I'm already giving my approval. Once you satisfied @orbeckst he can merge this PR/

kain88-de · 2018-07-20T07:57:54Z

package/MDAnalysis/analysis/dihedrals.py

+        ax.set(xticks=range(-180,181,60), yticks=range(-180,181,60),
+               xlabel=r"$\phi$ (deg)", ylabel=r"$\psi$ (deg)")
+        a = self.angles.reshape(np.prod(self.angles.shape[:2]), 2)
+        ax.scatter(a[:,0], a[:,1])


You need to add **kwargs to the scatter function as an argument.

kain88-de · 2018-07-20T07:58:16Z

testsuite/MDAnalysisTests/analysis/test_dihedrals.py

@@ -0,0 +1,39 @@
+import numpy as np


The copyright header should also be added here

pylint will also issue a warning here to add from __future__ import absolute_import as the top most import

codecov · 2018-07-20T19:46:58Z

Codecov Report

Merging #1997 into develop will increase coverage by 0.02%.
The diff coverage is 97.82%.

@@            Coverage Diff             @@
##           develop   #1997      +/-   ##
==========================================
+ Coverage    88.48%   88.5%   +0.02%     
==========================================
  Files          142     143       +1     
  Lines        17202   17249      +47     
  Branches      2635    2646      +11     
==========================================
+ Hits         15221   15267      +46     
  Misses        1385    1385              
- Partials       596     597       +1

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/dihedrals.py	`97.82% <97.82%> (ø)`
package/MDAnalysis/lib/distances.py	`87.22% <0%> (+0.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7cfc27...193e503. Read the comment docs.

kain88-de · 2018-07-20T20:10:05Z

package/MDAnalysis/analysis/dihedrals.py

+
+   import matplotlib.pyplot as plt
+   fig = plt.figure(figsize(5,5))
+   ax = fig.add_subplot(111)


a more concise way (and my preferred one)

fig, ax = plt.subplots()

It can also create a grid directly and change the figure size. Also, the line for creating a figure is not valid syntax.
I assume you change the figure size to get an equal aspect ratio. There is a better function called plt.figaspect you can use. As an argument, it accepts an aspect ratio and returns sensible figure sizes. So you can instead write

fig, ax = plt.subplots(figsize=plt.figaspect(1))

kain88-de · 2018-07-20T20:10:29Z

package/MDAnalysis/analysis/dihedrals.py

+can be accessed using :meth:`Ramachandran.angles`::
+
+   fig = plt.figure(figsize(5, 5))
+   ax = fig.add_subplot(111)


see comment above

kain88-de · 2018-07-20T20:11:49Z

package/MDAnalysis/analysis/dihedrals.py

+
+   from MDAnalysis.analysis.dihedrals import Ramachandran
+   R = Ramachandran(r)
+   R.run()


Preferred is R = Ramachandran(r).run() as a one-liner. The run method returns the object itself.

That's originally what I was doing but I've been using rms.py as a reference so I thought that was the convention. I'll be sure to change it back.

This might still be in older modules. We changed the return value of run only after the analysis class was introduced.

kain88-de · 2018-07-20T20:12:27Z

package/MDAnalysis/analysis/dihedrals.py

+   :members:
+   :inherited-members:
+
+   .. attribute:: angles


don't we want to have this as documentation in the class? @orbeckst is the information now rendered twice?

orbeckst · 2018-07-21T15:18:50Z

Attributes are not rendered automatically. You need to list them in the docs. However, if there is a Attributes section in the docs already then this one can be removed. Build the docs locally and visually check.

…

-- Oliver Beckstein email: orbeckst@gmail.com

Am Jul 20, 2018 um 16:12 schrieb Max Linke ***@***.***>: @kain88-de commented on this pull request. In package/MDAnalysis/analysis/dihedrals.py: > + ax.axis([-180,180,-180,180]) + ax.axhline(0, color='k', lw=1) + ax.axvline(0, color='k', lw=1) + ax.set(xticks=range(-180,181,60), yticks=range(-180,181,60), + xlabel=r"$\phi$ (deg)", ylabel=r"$\psi$ (deg)") + for ts in R.angles: + ax.scatter(ts[:,0], ts[:,1], color='k', marker='s') + +Analysis Class +-------------- + +.. autoclass:: Ramachandran + :members: + :inherited-members: + + .. attribute:: angles don't we want to have this as documentation in the class? @orbeckst is the information now rendered twice? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kain88-de · 2018-07-21T17:09:28Z

it's not in the class. I do link to have all docs in the class because it makes it easier to inspect documentation strings in an ipython session

…

On Sat, Jul 21, 2018 at 5:18 PM Oliver Beckstein ***@***.***> wrote: Attributes are not rendered automatically. You need to list them in the docs. However, if there is a Attributes section in the docs already then this one can be removed. Build the docs locally and visually check. -- Oliver Beckstein email: ***@***.*** > Am Jul 20, 2018 um 16:12 schrieb Max Linke ***@***.***>: > > @kain88-de commented on this pull request. > > In package/MDAnalysis/analysis/dihedrals.py: > > > + ax.axis([-180,180,-180,180]) > + ax.axhline(0, color='k', lw=1) > + ax.axvline(0, color='k', lw=1) > + ax.set(xticks=range(-180,181,60), yticks=range(-180,181,60), > + xlabel=r"$\phi$ (deg)", ylabel=r"$\psi$ (deg)") > + for ts in R.angles: > + ax.scatter(ts[:,0], ts[:,1], color='k', marker='s') > + > +Analysis Class > +-------------- > + > +.. autoclass:: Ramachandran > + :members: > + :inherited-members: > + > + .. attribute:: angles > don't we want to have this as documentation in the class? @orbeckst is the information now rendered twice? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub, or mute the thread. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1997 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEGnVh0Vj-GfAXNrD1v4MyE_2_Q754_7ks5uI0ZagaJpZM4VVHd5> .

kain88-de

This is the last thing I found

kain88-de · 2018-07-23T19:11:52Z

package/MDAnalysis/analysis/dihedrals.py

+    then a warning will be raised and they will be removed from the list of
+    residues, but the analysis will still run.
+
+    Run the analysis with :meth:`Ramachandran.run()`, whcih stores the results in the


Thanks I missed that. I'll probably go through the docstring one last time to catch anything else like that.

kain88-de · 2018-07-23T21:00:15Z

@orbeckst this should be done now

orbeckst · 2018-07-24T17:13:07Z

The linter is unhappy about something. Please fix and ping me. Or if @kain88-de is available and happy he can also merge. I currently only have limited access.

…

-- Oliver Beckstein email: orbeckst@gmail.com

Am Jul 23, 2018 um 17:00 schrieb Max Linke ***@***.***>: @orbeckst this should be done now — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

indicated the PR can be merged

kain88-de · 2018-07-25T05:46:37Z

Congratulations to your first PR

Henry Mull added 8 commits July 12, 2018 14:08

First commit of dihedrals.py

62a194f

added imports to dihedrals.py

30da783

fixed calculation errors and added plot function

1f149ad

Added docstring to dihedrals function

13ee8a0

updated plot function, simplified preparation

abbc3ef

Added IndexErrors to function

973910c

Updated residue selection of function, added test files

98c8370

Updated docstring of dihedrals.py

ad4e598

hfmull commented Jul 18, 2018

View reviewed changes

kain88-de requested changes Jul 18, 2018

View reviewed changes

richardjgowers reviewed Jul 18, 2018

View reviewed changes

orbeckst requested changes Jul 19, 2018

View reviewed changes

Henry Mull added 6 commits July 19, 2018 16:14

Switched to distances.calc_dihedrals and simplified plot function

4dabf32

Fixed imports

f0d8030

Added six to imports

15c2bd9

Fixed zip function

3b79336

Updated test references and added them to __all__

bfc75c9

Removed identical frames test, switched to assert_almost_equal

c5fcfb8

orbeckst previously requested changes Jul 20, 2018

View reviewed changes

kain88-de reviewed Jul 20, 2018

View reviewed changes

kain88-de approved these changes Jul 20, 2018

View reviewed changes

Updated docstrings of dihedrals.py, added files to docs

4812dc0

Updated CHANGELOG

610368e

kain88-de reviewed Jul 20, 2018

View reviewed changes

Fixed plotting in dihedrals.py, added more tests to test_dihedrals.py

60b1667

Fixed imports and docstring

e83bd9a

kain88-de requested changes Jul 23, 2018

View reviewed changes

Fixed docstring

7561232

kain88-de approved these changes Jul 23, 2018

View reviewed changes

Fixed imports of test_dihedral.py

193e503

kain88-de merged commit b50c6f0 into MDAnalysis:develop Jul 25, 2018

		@@ -0,0 +1,57 @@
		import numpy as np
		from numpy.testing import assert_array_almost_equal


		from MDAnalysis.analysis.base import AnalysisBase

		def dihedral_calc(atomgroups):

		@@ -415,5 +415,8 @@

		GSD = resource_filename(__name__, 'data/example.gsd')

		DihedralsArray = resource_filename(__name__, 'data/adk_oplsaa_dihedrals.npy')

New Dihedral and Ramachandran Analysis #1997

New Dihedral and Ramachandran Analysis #1997

Conversation

hfmull commented Jul 18, 2018 • edited Loading

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kain88-de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kain88-de commented Jul 18, 2018

orbeckst commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kain88-de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Jul 21, 2018 via email

kain88-de commented Jul 21, 2018 via email

kain88-de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kain88-de commented Jul 23, 2018

orbeckst commented Jul 24, 2018 via email

kain88-de commented Jul 25, 2018

hfmull commented Jul 18, 2018 •

edited

Loading

orbeckst Jul 19, 2018 •

edited

Loading

orbeckst left a comment •

edited

Loading

codecov bot commented Jul 20, 2018 •

edited

Loading