Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HydrogenBondAnalysis when no atom pair is with 3A #1325

Closed
wants to merge 12 commits into from
Closed

Fix HydrogenBondAnalysis when no atom pair is with 3A #1325

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
53e1d94
Fix hbond cannot deal with atoms too far
xiki-tempula Apr 28, 2017
b1f71e1
add change log
xiki-tempula Apr 28, 2017
dff9ad1
resolve conflict
xiki-tempula Apr 28, 2017
b6a8a07
resolve conflict
xiki-tempula Apr 28, 2017
c1d3eef
rebase
Apr 29, 2017
0f89262
Merge remote-tracking branch 'origin/h_bond' into h_bond
Apr 29, 2017
65536ea
Fix test and refine hbond analysis
Apr 29, 2017
98aba1f
Fix minor error and add py3 compatibility
Apr 29, 2017
cc947a4
change to static method
Apr 29, 2017
a44bbc5
Merge branch 'develop' into h_bond
orbeckst Apr 29, 2017
a9b238d
delete the file and lines
Apr 29, 2017
120f380
fix StringIO and static method
Apr 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package/CHANGELOG
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ Enhancements
Fixes
* In Universe.transfer_to_memory(): dt is now adjusted with step (Issue #1310)
* Various documentation sphinx errors (PR #1312)
* Fix hbond_analysis cannot deal with Universe where no two atoms are with 3A.
(PR #1325)
* Bugfix in confdistmatrix.get_distance_matrix; now works on all trajectory types.
(issue #1324)

Expand Down
2 changes: 2 additions & 0 deletions package/MDAnalysis/analysis/hbonds/hbond_analysis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,6 +823,8 @@ def _update_selection_2(self):
self._s2_donors = {}
self._s2_donors_h = {}
self._s2_acceptors = {}
if not self._s2:
return None
if self.selection1_type in ('donor', 'both'):
self._s2_acceptors = self._s2.select_atoms(
'name {0}'.format(' '.join(self.acceptors)))
Expand Down
18 changes: 17 additions & 1 deletion testsuite/MDAnalysisTests/analysis/test_hbonds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,12 @@

import itertools
import warnings
try:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?? I think it can be removed, can't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment. There is no StringIO in py3 so I was trying to make it compatible with py3. I still need some time to figure out how this stream reading works.

Copy link
Member

@orbeckst orbeckst Apr 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I didn't think about this. Then we should probably use six.StringIO: something like

from six import StringIO

which replaces

from StringIO import StringIO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. It seems that six is more powerful than I thought after spending an hour playing with py3.io.StringIO.

Copy link
Member

Choose a reason for hiding this comment

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

Use six.StringIO (as discussed in previous comment)

from BytesIO import BytesIO
except ImportError:
from io import BytesIO

from MDAnalysisTests.datafiles import PDB_helix, GRO, XTC
from MDAnalysisTests.datafiles import PDB_helix, GRO, XTC, HBond_atoms_far
Copy link
Member

Choose a reason for hiding this comment

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

remove HBond_atoms_far

# For type guessing:
from MDAnalysis.topology.core import guess_atom_type
from MDAnalysis.core.topologyattrs import Atomtypes
Expand Down Expand Up @@ -88,6 +92,18 @@ def test_generate_table(self):
assert_array_equal(h.table.acceptor_resnm, self.values['acceptor_resnm'])

@staticmethod
def test_atoms_too_far(self):
Copy link
Member

Choose a reason for hiding this comment

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

Minor thing: Make this a static method

@staticmethod
def test_atoms_too_far():
   ...

Copy link
Member

Choose a reason for hiding this comment

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

remove self from the argument list of the static method

pdb = '''TITLE Two atoms far away
CRYST1 52.763 52.763 52.763 90.00 90.00 90.00 P 1 1
MODEL 1
ATOM 1 N LEU 1 32.310 13.778 14.372 1.00 0.00 SYST N 0
ATOM 2 OW SOL 2 3.024 4.456 4.147 1.00 0.00 SYST H 0'''

u = MDAnalysis.Universe(BytesIO(pdb.encode()), format="pdb")
h = MDAnalysis.analysis.hbonds.HydrogenBondAnalysis(u, 'resname SOL', 'protein')
h.run(verbose=False)
assert_equal(h.timeseries, [[]])

Copy link
Member

Choose a reason for hiding this comment

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

There has to be a @staticmethod in front of the test_true_traj() function; Travis fails https://travis-ci.org/MDAnalysis/mdanalysis/jobs/227170225 , too, and QC flags it as a critical issue.

def test_true_traj():
Copy link
Member

Choose a reason for hiding this comment

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

This @staticmethod has to stay.

u = MDAnalysis.Universe(GRO, XTC)
u.add_TopologyAttr(guess_types(u.atoms.names))
Expand Down