Added zero-based indices for donors and acceptors in HBond Analysis. #833

jdetle · 2016-04-24T22:37:44Z

Changes made in this Pull Request:

Added references to 0-based indices in docs
Timeseries, counting by type, and timesteps by type now all have zero-based index
Added deprecation warning for 1-based indices upon calling HBondAnalysis
Recsql ids now also include zero-based indices even though they are redundant for uniqueness, figured that this would help when removing all references to 1-based indices later.
I went with a somewhat different name for 0-based indices than what was suggested in hbond analysis returns 1-based atom indices instead of canonical 0-based ones #807, just because it is shorter and I think it gets across the same thing. (donor_idx_zero instead of donor_atom_indices)

(Added versionchanged:: 0.15.0 4 times in different places saying essentially the same thing, I figured this was probably overkill but you could tell me where to delete extraneous stuff rather than where to add)

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

jdetle · 2016-04-24T23:19:27Z

I need to add tests to prove that this works as expected, and try to increase code coverage.

orbeckst · 2016-04-25T07:34:51Z

package/MDAnalysis/analysis/hbonds/hbond_analysis.py

-          8. "acceptor_atom"
-          9. "distance"
-          10. "angle"
+          3. "donor_idx_zero"


I don't like the "zero" in something that will eventually become the primary name. Can we go with donor_index and acceptor_index? I know that this is more confusing if you just look at the names but it will be better once we remove donor_idx and acceptor_idx.

Okay, will do.

updated docs to reflect change. Added deprecation warning for the 1-based index.

jdetle · 2016-05-03T03:40:36Z

Hi, I made some changes to the docs and added one simple test. Adding testing on real data is something I am working with but as of right now it is very slow. Are there small files to work with where we have some knowledge of what hydrogen bonds exist? Either way, I think this PR is ready for review.

samuel3181 · 2016-05-06T14:39:57Z

Hi @jdetle & @orbeckst
I have been following your efforts to fix the H-bond analysis defect. Does this mean that perhaps the version 15 of MDAnalysis would have this update?
thank you!
Sam

orbeckst · 2016-05-06T20:50:20Z

testsuite/MDAnalysisTests/analysis/test_hbonds.py

@@ -26,7 +26,7 @@
 import itertools
 import warnings

-from MDAnalysisTests.datafiles import PDB_helix
+from MDAnalysisTests.datafiles import PDB_helix, PSF, DCD


Are you using PSF, DCD anywhere for the tests?

orbeckst · 2016-05-06T20:51:23Z

@samuel3181 , the 0-based indices are almost ready to go in (unless @richardjgowers protests).
We don't have a fix for #801 yet.

orbeckst · 2016-05-06T20:53:49Z

Adding testing on real data is something I am working with but as of right now it is very slow. Are there small files to work with where we have some knowledge of what hydrogen bonds exist?

We don't have to look at whole trajectories. Just the first few frames can be sufficient. You can run a similar analysis with e.g. VMD and use that output as baseline truth. I think the original test looks for the i, i+4 protein backbone bonds in a alpha-helix.

jdetle · 2016-05-06T21:00:38Z

Okay, I'll finish this up tonight with that in mind. I have tests ready to
upload on my machine, just didn't want to add something stupidly slow.

On Fri, May 6, 2016 at 1:53 PM Oliver Beckstein notifications@github.com
wrote:

Adding testing on real data is something I am working with but as of right
now it is very slow. Are there small files to work with where we have some
knowledge of what hydrogen bonds exist?

We don't have to look at whole trajectories. Just the first few frames can
be sufficient. You can run a similar analysis with e.g. VMD and use that
output as baseline truth. I think the original test looks for the i, i+4
protein backbone bonds in a alpha-helix.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#833 (comment)

Have a wonderful day,
John J. Detlefs

Changed distance metrics to staticmethods Reworked selections to use implicit OR syntax Used direct bool comparison of Groups

Hbonds fixup

jdetle · 2016-05-08T03:06:27Z

@richardjgowers, do you know why the tests are failing now?

richardjgowers · 2016-05-08T09:38:31Z

@jdetle it was just the macos tests, so I'm blaming travis. I've restarted that set of tests

jdetle · 2016-05-08T22:53:17Z

Can you try again one more time @richardjgowers? Or could you tell me how to restart travis builds myself without pushing a new commit?

richardjgowers · 2016-05-09T08:12:55Z

@jdetle so around the top right of the travis webpage below "More options" there's a circular arrow type thingy that lets you restart a build (all jobs) or a single job (depending on where you click the thingy from)

kain88-de · 2016-05-09T08:29:07Z

I'm not sure if he is allowed to do that though.

richardjgowers · 2016-05-09T08:30:54Z

ah right yeah, I had to sign in first, good point.

jdetle · 2016-05-09T20:55:57Z

Any chance @MDAnalysis/coredevs could give me that level of access w/o giving me undue privileges? I think it would make things go smoother.

richardjgowers · 2016-05-11T15:38:14Z

@jdetle not entirely sure re: permissions. You should be able to use travis off your fork of MDA (it's free). If you go to the website you can sign in via github and add your repo of MDA so it'll work identically to ours

kain88-de · 2016-05-11T15:47:04Z

Random Travis failures are rare. Besides you can force a full rebuild by changing the SHA of the last commit.

jdetle force-pushed the hbonds branch from deeb092 to 05b7bb8 Compare April 24, 2016 22:41

orbeckst added Component-Analysis Work in progress labels Apr 25, 2016

orbeckst reviewed Apr 25, 2016
View reviewed changes

jdetle force-pushed the hbonds branch 2 times, most recently from 1fbf45d to 298ff51 Compare April 27, 2016 23:46

jdetle mentioned this pull request May 2, 2016

Opening universe described in HBondAnalysis docs causes error #838

Closed

jdetle added 3 commits May 2, 2016 20:37

Added zero-based indices for donors and acceptors in HBond Analysis,

ed9ece5

updated docs to reflect change. Added deprecation warning for the 1-based index.

Updated CHANGELOG

c231374

Updated tests, fixed some issues in docs

2bea8be

jdetle force-pushed the hbonds branch from 298ff51 to 2bea8be Compare May 3, 2016 03:38

Updated docs [skip ci]

9142426

orbeckst reviewed May 6, 2016
View reviewed changes

orbeckst added this to the 0.15.0 milestone May 6, 2016

orbeckst self-assigned this May 6, 2016

Added real trajectory test

58687ec

jdetle force-pushed the hbonds branch from 89ea208 to 58687ec Compare May 7, 2016 04:06

richardjgowers and others added 3 commits May 7, 2016 11:06

Fixed up test

3892406

Tidied hbond_analysis slightly

64596fc

Changed distance metrics to staticmethods Reworked selections to use implicit OR syntax Used direct bool comparison of Groups

Added real trajectory test

730bc3d

jdetle force-pushed the hbonds branch from 58687ec to 730bc3d Compare May 7, 2016 22:41

Merge pull request #1 from richardjgowers/hbonds

439901f

Hbonds fixup

richardjgowers merged commit b750c24 into MDAnalysis:develop May 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added zero-based indices for donors and acceptors in HBond Analysis. #833

Added zero-based indices for donors and acceptors in HBond Analysis. #833

jdetle commented Apr 24, 2016 •

edited

Loading

jdetle commented Apr 24, 2016

orbeckst Apr 25, 2016

jdetle Apr 25, 2016

jdetle commented May 3, 2016 •

edited

Loading

samuel3181 commented May 6, 2016

orbeckst May 6, 2016

orbeckst commented May 6, 2016

orbeckst commented May 6, 2016

jdetle commented May 6, 2016

jdetle commented May 8, 2016

richardjgowers commented May 8, 2016

jdetle commented May 8, 2016

richardjgowers commented May 9, 2016

kain88-de commented May 9, 2016

richardjgowers commented May 9, 2016

jdetle commented May 9, 2016

richardjgowers commented May 11, 2016

kain88-de commented May 11, 2016

Added zero-based indices for donors and acceptors in HBond Analysis. #833

Added zero-based indices for donors and acceptors in HBond Analysis. #833

Conversation

jdetle commented Apr 24, 2016 • edited Loading

PR Checklist

jdetle commented Apr 24, 2016

orbeckst Apr 25, 2016

Choose a reason for hiding this comment

jdetle Apr 25, 2016

Choose a reason for hiding this comment

jdetle commented May 3, 2016 • edited Loading

samuel3181 commented May 6, 2016

orbeckst May 6, 2016

Choose a reason for hiding this comment

orbeckst commented May 6, 2016

orbeckst commented May 6, 2016

jdetle commented May 6, 2016

jdetle commented May 8, 2016

richardjgowers commented May 8, 2016

jdetle commented May 8, 2016

richardjgowers commented May 9, 2016

kain88-de commented May 9, 2016

richardjgowers commented May 9, 2016

jdetle commented May 9, 2016

richardjgowers commented May 11, 2016

kain88-de commented May 11, 2016

jdetle commented Apr 24, 2016 •

edited

Loading

jdetle commented May 3, 2016 •

edited

Loading