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

vedantrathore · 2017-01-22T23:07:14Z

Part of #597

Changes made in this Pull Request:

Added unittest for validating errors
Added test for raising exceptions

PR Checklist

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

…/psa.py (part of MDAnalysis#597)

…to develop

vedantrathore · 2017-01-23T07:16:17Z

@kain88-de @jbarnoud can anyone please look into it?

jbarnoud

Thank you for tackling the issue. There are some changes to make, though.

In addition to the in-line comments, there are a few things that I would like the tests to look at:

dist_mat_to_vec issue a warning in some cases, this should be tested; look at assert_warns as it may help you to do so;
what if one of the parameter is negative?
what if N is null?

Testing the edge cases is very important. They are what breaks first, they also what can be broken in a first place. Therefore, testing them is crucial to find potential unexpected outputs.

jbarnoud · 2017-01-23T08:32:25Z

testsuite/MDAnalysisTests/analysis/test_psa.py

+    def test_dist_mat_to_vec_i_less_j(self):
+        """Test that distance is correct if i < j"""
+        err_msg = "dist_mat_to_vec function returning wrong values"
+        assert_(PSA.dist_mat_to_vec(5, 3, 4) is 9, err_msg)


The is statement tests identity, not equality. In the present case, you should use the == comparison; even better in that specific context would be to use assert_equal rather than assert_. With assert_equal the traceback is more informative as it tells you what value you got and what value you expected.

jbarnoud · 2017-01-23T08:33:13Z

testsuite/MDAnalysisTests/analysis/test_psa.py

+        assert_(PSA.dist_mat_to_vec(5, 3, 4) is 9, err_msg)
+
+    def test_dist_mat_to_vec_i_greater_j(self):
+        """Test that distance is correct if i < j"""


Copy-paste error. You forgot to reverse the comparison.

jbarnoud · 2017-01-23T08:35:28Z

testsuite/MDAnalysisTests/analysis/test_psa.py

@@ -93,6 +93,16 @@ 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 that distance is correct if i < j"""


The comment is misleading: dist_mat_to_vec does not return a distance, but an index. The docstring of the function says:

Convert distance matrix indices (in the upper triangle) to the index of
the corresponding distance vector.

jbarnoud · 2017-01-23T08:37:54Z

testsuite/MDAnalysisTests/analysis/test_psa.py

+        out of bounds of N"""
+
+        with self.assertRaises(ValueError):
+            PSA.dist_mat_to_vec(5, 6, 6)


Please, add cases with only i out of bonds, and with only j out of bonds.

jbarnoud · 2017-01-23T08:43:20Z

testsuite/MDAnalysisTests/analysis/test_psa.py

+        not Integers"""
+
+        with self.assertRaises(ValueError):
+            PSA.dist_mat_to_vec(5, '6', '7')


What if i, or j is a float?

vedantrathore · 2017-01-23T09:50:40Z

@jbarnoud Thanks I'll look into it

…to develop

…Part of MDAnalysis#597)

vedantrathore · 2017-01-23T11:14:06Z

@jbarnoud Is this okay? do tell me if something is wrong

jbarnoud

It is better, but not all the test cases are addressed.

jbarnoud · 2017-01-23T11:41:44Z

testsuite/MDAnalysisTests/analysis/test_psa.py

        out of bounds of N"""

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


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.

jbarnoud · 2017-01-23T11:42:13Z

testsuite/MDAnalysisTests/analysis/test_psa.py

@@ -138,7 +139,7 @@ def test_dist_mat_to_vec_func_bad_integers(self):

        with self.assertRaises(ValueError):
            PSA.dist_mat_to_vec(5, '6', '7')
-
+            PSA.dist_mat_to_vec(5, float(6), 7)


Same as above: separate the assertions.

jbarnoud · 2017-01-23T11:44:21Z

testsuite/MDAnalysisTests/analysis/test_psa.py

        out of bounds of N"""

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


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.

…Part of MDAnalysis#597)

vedantrathore · 2017-01-23T16:58:29Z

@jbarnoud is it okay now?

jbarnoud · 2017-01-23T17:01:20Z

It is OK for the in-line comments. I would like to see the edge cases I mentioned tested, though. Also, I would add one edge case to the list I made before: what if i == j?

…Part of MDAnalysis#597)

vedantrathore · 2017-01-23T17:07:01Z

@jbarnoud I think all the test cases are covered now?

jbarnoud · 2017-01-23T17:29:21Z

I do not see those corresponding to this comment: #1183 (review)

vedantrathore · 2017-01-23T18:14:17Z

@jbarnoud for the null and negative part, the function dist_mat_to_vec is not validating the inputs, so should I add a validation statement inside the function that raises ValueError when inputs is null or negative?

jbarnoud · 2017-01-24T08:04:07Z

It is not an issue if i or j equals 0. Yet, there seems to be edge cases where the function is not defined and that are not accounted for:

i == j where the function is not defined
i < 0 or j < 0

@sseyler I think you wrote the function, did you omit these cases on purpose? Do you think we should add the checks?

sseyler · 2017-01-25T00:43:22Z

@jbarnoud The couple of tests I included were there to quickly capture a couple of the possible erroneous inputs; they're definitely not exhaustive. I think you covered most of the corner cases, but to summarize (just in case), acceptable values of N, i, and j should be:

N ,i, and j integers
i > 0 and i < N-1 (i runs from 0 to N-2)
j > i and j < N (j runs from 1 to N-1)
N >= 2 (N is the length of a square distance matrix, so a distance vector is only defined when there are at least N = 2 objects to compare)

Assuming I haven't missed anything, all other values of N, i, and j don't make sense in this context. (So definitely unit test any cases falling outside of the above restrictions!)

By the way, thanks for the work on this, guys!

vedantrathore · 2017-01-26T09:37:05Z

so @jbarnoud should make any changes or just rebase my commits?

jbarnoud · 2017-01-26T09:51:16Z

@vedantrathore Well, writing the tests highlighted an issue with the code you were testing. Since the issue is small and easy to fix, it would be good to fix it in the same PR.

…to develop

vedantrathore · 2017-01-28T20:56:58Z

@jbarnoud do tell me if I added a wrong function. I think this function is already tested.

kain88-de · 2017-01-28T21:12:42Z

package/MDAnalysis/analysis/psa.py

+    try:
+        validate_dist_mat_to_vec_inputs(N, i, j)
+    except Exception as E:
+        raise E


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.

kain88-de · 2017-01-28T21:13:09Z

package/MDAnalysis/analysis/psa.py

+    :param N: int
+    :param i: int
+    :param j: int
+    :return: null


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

kain88-de · 2017-01-28T21:19:22Z

package/MDAnalysis/analysis/psa.py

+        iValue = int(i)
+        jValue = int(j)
+    except ValueError:
+        raise ValueError


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.

jbarnoud · 2017-01-29T09:20:02Z

@vedantrathore I think you went a bit overkill here. A couple of elifs in the existing chain would probably suffice.

The case where i == j is already accounted for as it falls in the else. The cases where i, and j are too large are tested, but the tests are wrong (i == N should not be valid). What is missing are the case where i or j is negative, the case where N < 2, and the case where one value is not an integer. I would skip the type checking as we never check types elsewhere.

Having a function to test the inputs of the function is not a bad idea. It can make the code clearer as testing the input is a different task as using it. If you choose to go for the separate function, then the validation function should do all the validation, leaving only the logic to the main function. Since the main function we have is quite specific in its error messages, the validation function should keep the specificity; this means at the very least raising the exception with an explanation. As a general rule, giving a explanation when raising an exception is rarely a bad idea. Finally, but this is just a matter of style and taste, I would prefix the name of the validation function with a _ to signal it is a "private" function that is not exposed in the API.

And just a last thing: playing a bit with dist_mat_to_vec, I noticed that there is a tiny bug in it. Line 586 from the version in develop is a print but should be a warning issued with warnings.warn (or should there be a message at all?). Also, the error message is missing the call to format to replace the placeholders with the values of the index. So line 586 should be:

warnings.warn(war_str.format(j, i))

This is why we need tests! Adding coverage for an handful of lines of code opened a can of worms. A small can, but a van still. @vedantrathore you picked well your function to test 😃

I let you come up with your solution. I have one in mind if you are stuck.

vedantrathore · 2017-01-29T12:48:05Z

@jbarnoud @kain88-de Thanks for the reviews, I'll take a look into it and rectify the error 😃

…Part of MDAnalysis#597)

…to develop

…Part of MDAnalysis#597)

vedantrathore · 2017-01-31T21:08:23Z

@jbarnoud Did I do it right? or should change something.

jbarnoud · 2017-02-01T11:52:00Z

testsuite/MDAnalysisTests/analysis/test_psa.py

+        with self.assertRaises(ValueError):
+            PSA.dist_mat_to_vec(5, -1, 2)
+
+        # Check if i is negative


Copy paste error. "Check if j is negative.

jbarnoud · 2017-02-01T11:57:15Z

package/MDAnalysis/analysis/psa.py

+    if i < 0 or j < 0 or N < 2:
+        error_str = "Matrix indices are invalid; i and j must be greater than 0 and N must be greater the 2"
+        raise ValueError(error_str)
+
    if i > N or j > N:


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)).

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

also the case where i or j is negative.

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.

jbarnoud · 2017-02-01T12:04:03Z

You are almost there! Beside the typo I pointed in an in-line comment, you have two things to do before I can merge:

The line that tests if the index are in bound is wrong. It would be nice to fix it (see in-line comment) and to account for the change in the tests.
You need to update package/CHANGELOG to say that the argument validation of dist_mat_to_vec is fixed (refer to this PR); you also need to add yourself as an author in package/CHANGELOG, package/AUTHORS, testsuite/CHANGELOG (no need to add an entry, though), and testsuite/AUTHORS.

Once this is fixed, I think it is all good (assuming the tests pass of course).

…Part of MDAnalysis#597)

vedantrathore · 2017-02-03T06:59:01Z

@kain88-de are all the requested changes completed? If so could you please merge this.

kain88-de · 2017-02-03T08:40:18Z

package/MDAnalysis/analysis/psa.py

@@ -573,7 +574,15 @@ def dist_mat_to_vec(N, i, j):
    :Returns:
      int, index (of the matrix element) in the corresponding distance vector
    """
-    if i > N or j > N:
+
+    if not (isinstance(N, int) or isinstance(i, int) or isinstance(j, int)):


Does this still work with numpy integers? So you pass np.int32(3) or np.int64(3) to the function?

@kain88-de Okay, thanks! I'll look into it

@kain88-de should I add diffrent clauses fr np.int32() and np.int64() and normal int?

One solution is to use isinstance(N, numbers.Integral).

…Part of MDAnalysis#597)

vedantrathore · 2017-02-03T08:58:55Z

@kain88-de Is this fine?

kain88-de · 2017-02-03T09:05:12Z

package/MDAnalysis/analysis/psa.py

@@ -575,7 +575,7 @@ def dist_mat_to_vec(N, i, j):
      int, index (of the matrix element) in the corresponding distance vector
    """

-    if not (isinstance(N, int) or isinstance(i, int) or isinstance(j, int)):
+    if not (isinstance(N, numbers.Integral) or isinstance(i, numbers.Integral) or isinstance(j, numbers.Integral)):


you should add tests as well for the numpy integers

Oh forgot that, my bad.

@kain88-de I was thinking about how to check whether input is numpy integer or not, I think we are only checking whether the input is not an integer, How should I validate whether the input is valid integer?

The current check is testing if the input is an integer. In you test you should just make sure that it's ok to use numpy integers like np.int32 in the function. That is important if one wants to use integers from numpy arrays.

What @kain88-de means is that you should add a few lines in test_psa.py with numpy integer as inputs. The function should work of we give N=numpy.int32(4) or N=numpy.int16(4). and this has to be tested. It is likely that the input values for dist_mat_to_vec will come from a numpy array at some point, then their type will not be a plain int.

@kain88-de @jbarnoud I think this is what you meant, right?

…Part of MDAnalysis#597)

vedantrathore · 2017-02-03T21:07:35Z

@kain88-de I think It's done now, all the tests are passed.

kain88-de · 2017-02-04T07:22:37Z

Thanks

vedantrathore · 2017-02-04T16:43:13Z

@jbarnoud @kain88-de I would like to complete this as my gsoc project. Any guidance on how to start with it?

kain88-de · 2017-02-04T18:18:01Z

You can write us on the mailing list or the referenced bug to discuss this idea.

orbeckst · 2017-02-07T08:09:20Z

This merge introduced a ton of commits with identical messages and made the history look ugly/confusing. @kain88-de when you merge a PR like this the next time please squash/merge everything or ask for it to be rebased/squashed by the contributor. Thanks.

@MDAnalysis/coredevs should we rebase develop and squash all of the commits belonging to this PR, the force push? (In order to do this, branch protection has to be temporarily lifted.)

jbarnoud · 2017-02-07T08:16:41Z

I would avoid force pushing on develop. Even though there are several commits with identical titles, at least they are identical, grouped in time, and meaningful. IMHO it is not harmful enough in the history to create a precedent of lifting the branch protection.

vedantrathore added 2 commits January 23, 2017 04:27

Added exception and unittest for function dist_mat_to_vec of analysis…

9a49244

…/psa.py (part of MDAnalysis#597)

Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…

ceb1618

…to develop

jbarnoud requested changes Jan 23, 2017

View reviewed changes

vedantrathore added 2 commits January 23, 2017 16:36

Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…

1b04205

…to develop

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…

3af71be

…Part of MDAnalysis#597)

jbarnoud requested changes Jan 23, 2017

View reviewed changes

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…

bf8765b

…Part of MDAnalysis#597)

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…

cda6c57

…Part of MDAnalysis#597)

vedantrathore added 2 commits January 29, 2017 02:23

Adds validation for dist_mat_to_vec function inputs

dcc6168

Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…

cecc350

…to develop

kain88-de requested changes Jan 28, 2017

View reviewed changes

vedantrathore added 3 commits February 1, 2017 02:33

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…

3943ddc

…Part of MDAnalysis#597)

Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…

b706493

…to develop

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…

f12458e

…Part of MDAnalysis#597)

jbarnoud reviewed Feb 1, 2017

View reviewed changes

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…

ece534a

…Part of MDAnalysis#597)

jbarnoud approved these changes Feb 2, 2017

View reviewed changes

kain88-de requested changes Feb 3, 2017

View reviewed changes

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…

36acf58

…Part of MDAnalysis#597)

kain88-de requested changes Feb 3, 2017

View reviewed changes

vedantrathore added 2 commits February 3, 2017 16:36

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…

376b0b1

…Part of MDAnalysis#597)

Added exception and unittest for dist_mat_to_vec of analysis/psa.py (…

bd94007

…Part of MDAnalysis#597)

kain88-de approved these changes Feb 4, 2017

View reviewed changes

kain88-de merged commit da70c1f into MDAnalysis:develop Feb 4, 2017

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

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

Conversation

vedantrathore commented Jan 22, 2017 • edited by kain88-de Loading

PR Checklist

vedantrathore commented Jan 23, 2017

jbarnoud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedantrathore commented Jan 23, 2017

vedantrathore commented Jan 23, 2017

jbarnoud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedantrathore commented Jan 23, 2017

jbarnoud commented Jan 23, 2017

vedantrathore commented Jan 23, 2017

jbarnoud commented Jan 23, 2017

vedantrathore commented Jan 23, 2017

jbarnoud commented Jan 24, 2017

sseyler commented Jan 25, 2017

vedantrathore commented Jan 26, 2017

jbarnoud commented Jan 26, 2017

vedantrathore commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbarnoud commented Jan 29, 2017

vedantrathore commented Jan 29, 2017

vedantrathore commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedantrathore Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbarnoud commented Feb 1, 2017

vedantrathore commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedantrathore commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedantrathore commented Feb 3, 2017

kain88-de commented Feb 4, 2017

vedantrathore commented Feb 4, 2017

kain88-de commented Feb 4, 2017

orbeckst commented Feb 7, 2017

jbarnoud commented Feb 7, 2017

vedantrathore commented Jan 22, 2017 •

edited by kain88-de

Loading

vedantrathore Feb 2, 2017 •

edited

Loading