From 7051349fcbc83346d172661c29e92c30bef6eaf3 Mon Sep 17 00:00:00 2001 From: Jonathan Barnoud Date: Tue, 17 Jan 2017 11:48:01 +0100 Subject: [PATCH 1/3] lib.util.Namespace raises the expected exception `lib.util.Namespace` allows to query a namespace by index or by attribute. It allows to use both the following syntaxes to access the same element: ns = lib.util.Namespace() ns['key'] = 'plop' ns['key'] # Access by index ns.key # Access by attribute The class is used to permit some flexibility when accessing auxiliary data attached to trajectories. Prior to this commit, accessing a non-existing item from a namespace by attribute would raise a KeyError while an AttributeError should be raise. This breaks functions that test the features of an object by looking for some attributes, and are therefore capturing AttributeError as a sign the attribute is missing. In particular, raising the wrong exception confused `numpy.lib.type_check.iscomplexobj` in numpy 1.12.0. This confusion lead to `assert_equal` to miss behave in that version of numpy, therefore breaking the tests for MDAnalysis when upgrading from python 1.11.3 to 1.12.0. This commit makes sure that the expected exception is raised when accessing an non-existing item by attribute in a Namespace object. --- package/CHANGELOG | 2 ++ package/MDAnalysis/lib/util.py | 25 ++++++++++++++++--- testsuite/MDAnalysisTests/coordinates/base.py | 4 +-- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/package/CHANGELOG b/package/CHANGELOG index 56668ce06b9..ab544e1fba3 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -91,6 +91,8 @@ Fixes * Fixed selections using operators backwards ('prop 10 > mass') and sensitivity about whitespace around these (PR #1156 Issue #1011 #1009) * Fixed PSA analysis is now using AlignTraj + * The namespace used for auxiliary data now raises an AttributeError instead + of a KeyError when accessing data by attributes (Issue #1166) Changes * Started unifying the API of analysis classes (named internally diff --git a/package/MDAnalysis/lib/util.py b/package/MDAnalysis/lib/util.py index 3596be4518f..7361aae06f6 100644 --- a/package/MDAnalysis/lib/util.py +++ b/package/MDAnalysis/lib/util.py @@ -1335,12 +1335,27 @@ class Namespace(object): """Class to allow storing attributes in new namespace. """ def __getattr__(self, key): # a.this causes a __getattr__ call for key = 'this' - return self.__dict__[key] + try: + return self.__dict__[key] + except KeyError: + raise AttributeError('"{}" is not known in the namespace.' + .format(key)) + def __setattr__(self, key, value): # a.this = 10 causes a __setattr__ call for key='this' value=10 - self.__dict__[key] = value + try: + self.__dict__[key] = value + except KeyError: + raise AttributeError('"{}" is not known in the namespace.' + .format(key)) + def __delattr__(self, key): - del self.__dict__[key] + try: + del self.__dict__[key] + except KeyError: + raise AttributeError('"{}" is not known in the namespace.' + .format(key)) + def __eq__(self, other): try: # this'll allow us to compare if we're storing arrays @@ -1348,12 +1363,16 @@ def __eq__(self, other): except AssertionError: return False return True + def __str__(self): return str(self.__dict__) + def __len__(self): return len(self.__dict__) + def __getitem__(self, key): return self.__dict__[key] + def __iter__(self): for i in self.__dict__: yield i diff --git a/testsuite/MDAnalysisTests/coordinates/base.py b/testsuite/MDAnalysisTests/coordinates/base.py index 1c01aeac012..1f23f041f32 100644 --- a/testsuite/MDAnalysisTests/coordinates/base.py +++ b/testsuite/MDAnalysisTests/coordinates/base.py @@ -293,7 +293,7 @@ def test_add_same_auxname_raises_ValueError(self): def test_remove_auxiliary(self): self.reader.remove_auxiliary('lowf') assert_raises(AttributeError, getattr, self.reader._auxs, 'lowf') - assert_raises(KeyError, getattr, self.reader.ts.aux, 'lowf') + assert_raises(AttributeError, getattr, self.reader.ts.aux, 'lowf') @raises(ValueError) def test_remove_nonexistant_auxiliary_raises_ValueError(self): @@ -344,7 +344,7 @@ def test_rename_aux(self): assert_equal(self.reader.ts.aux.lowf_renamed, self.ref.aux_lowf_data[0]) # old name should be removed - assert_raises(KeyError, getattr, self.reader.ts.aux, 'lowf') + assert_raises(AttributeError, getattr, self.reader.ts.aux, 'lowf') # new name should be retained next(self.reader) assert_equal(self.reader.ts.aux.lowf_renamed, From 149037c7f9754265a7954c87758d872e5db7bc3f Mon Sep 17 00:00:00 2001 From: Jonathan Barnoud Date: Tue, 17 Jan 2017 12:46:26 +0100 Subject: [PATCH 2/3] Remove unnecessary mention in the CHANGELOG --- package/CHANGELOG | 2 -- 1 file changed, 2 deletions(-) diff --git a/package/CHANGELOG b/package/CHANGELOG index ab544e1fba3..56668ce06b9 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -91,8 +91,6 @@ Fixes * Fixed selections using operators backwards ('prop 10 > mass') and sensitivity about whitespace around these (PR #1156 Issue #1011 #1009) * Fixed PSA analysis is now using AlignTraj - * The namespace used for auxiliary data now raises an AttributeError instead - of a KeyError when accessing data by attributes (Issue #1166) Changes * Started unifying the API of analysis classes (named internally From 83683c9d338e6898b7d633408e73ecaa2db86e3d Mon Sep 17 00:00:00 2001 From: Richard Gowers Date: Wed, 18 Jan 2017 12:14:09 +0000 Subject: [PATCH 3/3] Fixed lib.util.Namespace setattr Added tests for lib.util.Namespace Moved test_util to lib/test_util --- package/MDAnalysis/lib/util.py | 28 ++----- .../MDAnalysisTests/{ => lib}/test_util.py | 77 +++++++++++++++++++ 2 files changed, 82 insertions(+), 23 deletions(-) rename testsuite/MDAnalysisTests/{ => lib}/test_util.py (94%) diff --git a/package/MDAnalysis/lib/util.py b/package/MDAnalysis/lib/util.py index 7361aae06f6..1d5b92ae203 100644 --- a/package/MDAnalysis/lib/util.py +++ b/package/MDAnalysis/lib/util.py @@ -1331,27 +1331,22 @@ def blocks_of(a, n, m): return np.lib.stride_tricks.as_strided(a, new_shape, new_strides) -class Namespace(object): +class Namespace(dict): """Class to allow storing attributes in new namespace. """ def __getattr__(self, key): # a.this causes a __getattr__ call for key = 'this' try: - return self.__dict__[key] + return dict.__getitem__(self, key) except KeyError: raise AttributeError('"{}" is not known in the namespace.' .format(key)) def __setattr__(self, key, value): - # a.this = 10 causes a __setattr__ call for key='this' value=10 - try: - self.__dict__[key] = value - except KeyError: - raise AttributeError('"{}" is not known in the namespace.' - .format(key)) + dict.__setitem__(self, key, value) def __delattr__(self, key): try: - del self.__dict__[key] + dict.__delitem__(self, key) except KeyError: raise AttributeError('"{}" is not known in the namespace.' .format(key)) @@ -1359,20 +1354,7 @@ def __delattr__(self, key): def __eq__(self, other): try: # this'll allow us to compare if we're storing arrays - assert_equal(self.__dict__, other.__dict__) + assert_equal(self, other) except AssertionError: return False return True - - def __str__(self): - return str(self.__dict__) - - def __len__(self): - return len(self.__dict__) - - def __getitem__(self, key): - return self.__dict__[key] - - def __iter__(self): - for i in self.__dict__: - yield i diff --git a/testsuite/MDAnalysisTests/test_util.py b/testsuite/MDAnalysisTests/lib/test_util.py similarity index 94% rename from testsuite/MDAnalysisTests/test_util.py rename to testsuite/MDAnalysisTests/lib/test_util.py index f1490f04242..bbd5e30fe42 100644 --- a/testsuite/MDAnalysisTests/test_util.py +++ b/testsuite/MDAnalysisTests/lib/test_util.py @@ -819,3 +819,80 @@ def test_blocks_of_VE(self): arr = np.arange(16).reshape(4, 4) assert_raises(ValueError, util.blocks_of, arr, 2, 1) + + +class TestNamespace(object): + def setUp(self): + self.ns = util.Namespace() + + def tearDown(self): + del self.ns + + def test_getitem(self): + self.ns.this = 42 + + assert_(self.ns['this'] == 42) + + def test_getitem_KE(self): + assert_raises(KeyError, dict.__getitem__, self.ns, 'this') + + def test_setitem(self): + self.ns['this'] = 42 + + assert_(self.ns['this'] == 42) + + def test_delitem(self): + self.ns['this'] = 42 + assert_('this' in self.ns) + del self.ns['this'] + assert_(not ('this' in self.ns)) + + def test_delitem_AE(self): + def deller(): + del self.ns.this + assert_raises(AttributeError, deller) + + def test_setattr(self): + self.ns.this = 42 + + assert_(self.ns.this == 42) + + def test_getattr(self): + self.ns['this'] = 42 + + assert_(self.ns.this == 42) + + def test_getattr_AE(self): + assert_raises(AttributeError, getattr, self.ns, 'this') + + def test_delattr(self): + self.ns['this'] = 42 + + assert_('this' in self.ns) + del self.ns.this + assert_(not ('this' in self.ns)) + + def test_eq(self): + self.ns['this'] = 42 + + ns2 = util.Namespace() + ns2['this'] = 42 + + assert_(self.ns == ns2) + + def test_len(self): + assert_(len(self.ns) == 0) + self.ns['this'] = 1 + self.ns['that'] = 2 + assert_(len(self.ns) == 2) + + def test_iter(self): + self.ns['this'] = 12 + self.ns['that'] = 24 + self.ns['other'] = 48 + + seen = [] + for val in self.ns: + seen.append(val) + for val in ['this', 'that', 'other']: + assert_(val in seen)