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.

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

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (Part of #597) #1183

Merged
merged 15 commits into from
Feb 4, 2017
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
9a49244
Added exception and unittest for function dist_mat_to_vec of analysis…
vedantrathore Jan 22, 2017
ceb1618
Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…
vedantrathore Jan 22, 2017
1b04205
Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…
vedantrathore Jan 23, 2017
3af71be
Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…
vedantrathore Jan 23, 2017
bf8765b
Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…
vedantrathore Jan 23, 2017
cda6c57
Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…
vedantrathore Jan 23, 2017
dcc6168
Adds validation for dist_mat_to_vec function inputs
vedantrathore Jan 28, 2017
cecc350
Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…
vedantrathore Jan 28, 2017
3943ddc
Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…
vedantrathore Jan 31, 2017
b706493
Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…
vedantrathore Jan 31, 2017
f12458e
Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…
vedantrathore Jan 31, 2017
ece534a
Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…
vedantrathore Feb 2, 2017
36acf58
Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…
vedantrathore Feb 3, 2017
376b0b1
Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…
vedantrathore Feb 3, 2017
bd94007
Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…
vedantrathore Feb 3, 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
28 changes: 28 additions & 0 deletions package/MDAnalysis/analysis/psa.py
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,10 @@ def dist_mat_to_vec(N, i, j):
:Returns:
int, index (of the matrix element) in the corresponding distance vector
"""
try:
validate_dist_mat_to_vec_inputs(N, i, j)
except Exception as E:
raise E
Copy link
Member

Choose a reason for hiding this comment

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

If you don't want to catch the exception and to some special error handling with it then you don't need the try/except block. You can remove it here.

if i > N or j > N:
Copy link
Contributor

Choose a reason for hiding this comment

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

According to @sseyler comment, this test should be i > N - 1 or j > N. Yet, because the function knows how to look at the lower triangle too, it should be (j > i and (i > N - 1 or j > N)) or (j < i and (i > N or j > N - 1)).

Copy link
Contributor Author

@vedantrathore vedantrathore Feb 2, 2017

Choose a reason for hiding this comment

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

@jbarnoud, but this doesn't include the case in which N > 2, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also the case where i or j is negative.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am only talking about replacing the line that says if i > N or j > N:. The value of N, and i and j being negative, are tested with the previous if.

err_str = "Matrix indices are out of range; i and j must be less than" \
+ " N = {0:d}".format(N)
Expand All @@ -591,6 +595,30 @@ def dist_mat_to_vec(N, i, j):
raise ValueError(err_str)


def validate_dist_mat_to_vec_inputs(N, i, j):
"""
Validate the inputs of the function dist_mat_to_vec else raise an exception
:param N: int
:param i: int
:param j: int
:return: null
Copy link
Member

Choose a reason for hiding this comment

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

We follow numpy style docs in new code. You can have a look into other files for the look.

"""

# Validating if the inputs are integer
try:
NValue = int(N)
iValue = int(i)
jValue = int(j)
except ValueError:
raise ValueError
Copy link
Member

Choose a reason for hiding this comment

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

Integer evaluation isn't done like that. Here we just check if the input can be converted to an integer. So '1' would still be a valid input here with this code. The correct test is isinstance(var, [class]). So here it would be

if not (isinstance(N, int) or isinstance(i, int) or isinstance(j, int)):
    raise ValueError("N,i,j all must be of type int")

Here you should also check if this still works with numpy integer types.


# Check if the inputs are not out of bounds
if N-1 > i > 0 and N >= 2 and i < j < N:
pass
else:
raise ValueError


class Path(object):
"""Pre-process a :class:`MDAnalysis.Universe` object: (1) fit the
trajectory to a reference structure, (2) convert fitted time series to a
Expand Down
47 changes: 44 additions & 3 deletions testsuite/MDAnalysisTests/analysis/test_psa.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@

from numpy.testing import (TestCase, dec, assert_array_less,
assert_array_almost_equal, assert_,
assert_almost_equal)
assert_almost_equal, assert_equal)
import numpy as np

from MDAnalysisTests.datafiles import PSF, DCD, DCD2
Expand Down Expand Up @@ -93,6 +93,15 @@ def test_dendrogram_produced(self):
err_msg = "Dendrogram dictionary object was not produced"
assert_(type(self.plot_data[1]) is dict, err_msg)

def test_dist_mat_to_vec_i_less_j(self):
"""Test the index of corresponding distance vector is correct if i < j"""
err_msg = "dist_mat_to_vec function returning wrong values"
assert_equal(PSA.dist_mat_to_vec(5, 3, 4), 9, err_msg)

def test_dist_mat_to_vec_i_greater_j(self):
"""Test the index of corresponding distance vector is correct if i > j"""
err_msg = "dist_mat_to_vec function returning wrong values"
assert_equal(PSA.dist_mat_to_vec(5, 4, 3), 9, err_msg)

class TestPSAExceptions(TestCase):
'''Tests for exceptions that should be raised
Expand All @@ -108,13 +117,45 @@ def test_get_path_metric_func_bad_key(self):
self.fail('KeyError should be caught')

def test_get_coord_axes_bad_dims(self):
'''Test that ValueError is raised when
"""Test that ValueError is raised when
numpy array with incorrect dimensions
is fed to get_coord_axes().'''
is fed to get_coord_axes()."""

with self.assertRaises(ValueError):
PSA.get_coord_axes(np.zeros((5,5,5,5)))

def test_dist_mat_to_vec_func_out_of_bounds(self):
"""Test that ValueError is raised when i or j or both are
out of bounds of N"""

# Check if i is out of bounds of N
with self.assertRaises(ValueError):
PSA.dist_mat_to_vec(5, 6, 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have each of these line in a different with block, or in a different test. The way it is now makes it difficult to now which one failed if there is a failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment to tell what index is out of bonds. It is obvious when you are in the context of writing the test, but it makes you loose quite a lot of time when you have to figure it out latter on.


# Check if j is out of bounds of N
with self.assertRaises(ValueError):
PSA.dist_mat_to_vec(5, 4, 6)

# Check if both i and j are out of bounds of N
with self.assertRaises(ValueError):
PSA.dist_mat_to_vec(5, 6, 7)

def test_dist_mat_to_vec_func_i_equals_j(self):
"""Test that ValueError is raised when i == j"""

with self.assertRaises(ValueError):
PSA.dist_mat_to_vec(5, 4, 4)

def test_dist_mat_to_vec_func_bad_integers(self):
"""Test that ValueError is raised when i or j are
not Integers"""

with self.assertRaises(ValueError):
PSA.dist_mat_to_vec(5, '6', '7')
Copy link
Contributor

Choose a reason for hiding this comment

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

What if i, or j is a float?


with self.assertRaises(ValueError):
PSA.dist_mat_to_vec(5, float(6), 7)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: separate the assertions.


class _BaseHausdorffDistance(TestCase):
'''Base Class setup and unit tests
for various Hausdorff distance
Expand Down