AttributeError when using MDAnalysis.analysis.hydrogenbonds.hbond_analysis with topology file without bonds information

[VOD555]

Expected behavior

When given a universe without bonds attribute, should raise Exception

  Cannot assign donor-hydrogen pairs via topology as no bonded information is present.                           
  Please either: load a topology file with bonded information; use the guess_bonds() 
  topology guesser; or set HydrogenBondAnalysis.donors_sel so that a distance cutoff                              
  can be used.

Actual behavior

And AttributeError was raised:

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
~/anaconda3/envs/py36/lib/python3.6/site-packages/MDAnalysis/core/universe.py in __getattr__(self, key)
    509         try:
--> 510             segment = self._instant_selectors[key]
    511         except KeyError:

KeyError: 'bonds'

During handling of the above exception, another exception occurred:

AttributeError                            Traceback (most recent call last)
<ipython-input-12-763dc8d4b193> in <module>
----> 1 gro.bonds

~/anaconda3/envs/py36/lib/python3.6/site-packages/MDAnalysis/core/universe.py in __getattr__(self, key)
    510             segment = self._instant_selectors[key]
    511         except KeyError:
--> 512             raise AttributeError('No attribute "{}".'.format(key))
    513         else:
    514             warnings.warn("Instant selector Universe.<segid> "

AttributeError: No attribute "bonds".

Code to reproduce the behavior

Show us how to reproduce the failiure. If you can, use trajectory files from the test data.

import MDAnalysis
from MDAnalysis.analysis.hydrogenbonds.hbond_analysis import HydrogenBondAnalysis
from MDAnalysisTests.datafiles import GRO

kwargs = {
        'donors_sel': None,
        'hydrogens_sel': None,
        'acceptors_sel': None,
        'd_h_cutoff': 1.2,
        'd_a_cutoff': 3.0,
        'd_h_a_angle_cutoff': 120.0
        }

u = MDAnalysis.Universe(GRO)
h = HydrogenBondAnalysis(u, **kwargs)
h._get_dh_pairs()

Currently version of MDAnalysis

• Which version are you using?
  0.21.0
• Which version of Python?
  3.6.9

[orbeckst]

PR MDAnalysis/pmda#113 has code that can be used to properly raise an exception.

[bieniekmateusz]

We also corrected it the same way with @p-j-smith after seeing this problem. I think @p-j-smith is more suitable to be an assignee here!

[orbeckst]

I had assigned it to you because PR #2572 seemed to be addressing it ... and your icon popped up first.

[p-j-smith]

This is indeed fixed in PR #2572 and we've added a test for it too

[orbeckst]

Cool, thanks. So we’ll just wait for that PR to be merged.

[zemanj]

Maybe related:
In #2565, the goal is to turn AttributeErrors into more meaningful NoDataErrors stating that the required info is missing from the topology. Nevertheless, I think that hbond_analysis deserves its own error message as suggested above, so one should explicitly test for bond presence in this case.

Caveats when checking for bond presence:
Note that hasattr(u, 'bonds') is an incredibly slow test because it involves a lot of object instantiations under the hood. hasattr(u._topology, 'bonds') is much faster:

import MDAnalysis as mda
from MDAnalysis.tests.datafiles import GRO, TPR

u = mda.Universe(GRO)  # universe *without* bonds
%timeit hasattr(u, 'bonds')
# 758 µs ± 4.33 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
%timeit hasattr(u._topology, 'bonds')
# 216 ns ± 2.88 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

u = mda.Universe(TPR)  # universe *with* bonds
%timeit hasattr(u, 'bonds')
# 88.8 ms ± 524 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)   <== AAARGH!
%timeit hasattr(u._topology, 'bonds')
# 86.4 ns ± 0.266 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

You see that in the latter example, hasattr(u, 'bonds') is a million times slower than hasattr(u._topology, 'bonds'). This may be irrelevant for a setup routine that is run only once, but when being called in a loop, hasattr(u, 'bonds') is a nice way of very thoroughly killing performance. 😁