Issue 1006: minor fixes and several new unit tests for PSA module #1508

sseyler · 2017-07-18T01:46:30Z

Addresses Issue #1006 (increase code coverage in PSA) and related Issue #1507 (Some PSA module components don't behave as expected). Brings PSA code coverage up to 73% according to report from mda_nosetests:

./mda_nosetests -v --with-coverage --cover-erase --cover-package=MDAnalysis.analysis.psa analysis/test_psa.py

gives:

Name                         Stmts   Miss  Cover
------------------------------------------------
MDAnalysis/analysis/psa.py     530    143    73%

Changes made in this Pull Request:

MDAnalysis.analysis.psa.dist_mat_to_vec() now returns int type via explicit casting of return statements (Some PSA module components don't behave as expected #1507).
The Hausdorff pairs analysis, run_pairs_analysis(), now also gives the distance matrix, as with the simpler run() method, by default (Some PSA module components don't behave as expected #1507).
- Also fixes a small issue in storing the distance matrix when a Python function is provided (e.g., psa.run(metric=PSA.hausdorff) versus psa.run(metric='hausdorff') as a kwarg and a filename isn't explicitly specified to psa.run().
Added 3 new unit tests applicable to PSAnalysis class (increase code coverage in PSA #1006)
1. test_explicit_metric()—check that an explicit Python function reference, as opposed to a string specifier, works as keyword argument, i.e., psa.run(metric=psa.hausdorff) versus psa.run(metric='hausdorff')
2. test_hausdorff_pairs_distances()—check that Hausdorff distances produced by Hausdorff pairs analysis from psa.run_pairs_analysis(neighbors=True, hausdorff_pairs=True) are equal to Hausdorff distances generated in the usual way with psa.run()
3. test_distances_from_hausdorff_pairs_frames()—check, for all pairs of paths, that the actual distance (computed manually by utilizing psa.sqnorm()) between two frames corresponding to a Hausdorff pair is equal to the Hausdorff distance between the two corresponding paths.

PR Checklist

Tests
Docs?
CHANGELOG updated?
Issue raised/referenced?

sseyler · 2017-07-18T02:05:43Z

I just checked that the changes I made to the return statements in psa.dist_mat_to_vec() on lines 738 and 744 (undoing the explicit cast to int type) still give passing tests:

line 738: return int( (N*i) + j - (i+2)*(i+1)/2 ) --> return (N*i) + j - (i+2)*(i+1)/2
line 744: return int( (N*j) + i - (j+2)*(j+1)/2 ) --> return (N*j) + i - (j+2)*(j+1)/2

... and it appears that it doesn't:

======================================================================
ERROR: Test whether actual distances between frames of Hausdorff
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/nfs/homes3/sseyler/Repositories/python/mdanalysis/testsuite/MDAnalysisTests/analysis/test_psa.py", line 157, in test_distances_from_hausdorff_pairs_frames
    p, q = self.hausd_pairs[pairidx]['frames']
TypeError: list indices must be integers, not float

I will look more closely next chance I get to see if what the problem is (or if the problem is me).

richardjgowers · 2017-07-18T16:32:06Z

@sseyler it might be the division causing floats?

kain88-de · 2017-07-18T16:35:03Z

@sseyler you can enforce the old style in division using double backslashes a // n.

sseyler · 2017-07-18T17:59:11Z

@richardjgowers @kain88-de

Yep, that was the problem. Using double backslashes now.

orbeckst · 2017-07-18T18:28:59Z

(Totally nit-picking here but the character / is called a (forward) slash; the backslash is the character \.)

Thanks for working on the coverage!

orbeckst · 2017-07-18T18:31:24Z

Btw, you should rebase against the latest develop: there is no nose test anymore #884 . Make sure that this works with the latest and greatest.

richardjgowers · 2017-07-18T22:02:18Z

@sseyler this is failing because of the linter. Add from __future__ import division at the top, and from six.moves import range and use range instead of xrange

…_wavg_weight() and test_asymmetric_avg_weight(); need diff names for each test.

…est names: test_asymmetric_weight()

…now also gives distance matrix

…lysis docs for dist matrix D

…tput

- Added future division and six.range to test_psa.py - Separated pairs analysis (_run_pairs_analysis) from _run; moved plotting before - Adding a couple distance matrix checks to PSAnalysis methods - PSAnalysis.cluster now uses generated distance matrix D by default and checks for proper dist matrix format if dist_mat kwarg is specified - updated docstring for PSAnalysis.cluster

richardjgowers · 2017-07-20T22:52:35Z

package/MDAnalysis/analysis/psa.py

@@ -254,6 +254,10 @@ def get_path_metric_func(name):
    except KeyError as key:
        print("Path metric {0} not found. Valid selections: ".format(key))


Shouldn't this raise a KeyError?

Maybe change it to a value error. But I would keep the exception. An invalid selection does sound like a grave mistake

richardjgowers · 2017-07-20T22:53:09Z

package/MDAnalysis/analysis/psa.py

-            filename = kwargs.pop('filename', str(metric))
-            self.save_result(filename=filename)
+            filename = kwargs.pop('filename', metric)
+            if type(metric) is str:


if isinstance(metrix, six.string_types)

orbeckst · 2017-07-20T23:26:56Z

Please no print(). Use logger and warning or possibly just raise the damn exception and let the user do things properly. Failing is a good thing.

In package/MDAnalysis/analysis/psa.py <#1508 (comment)>: > @@ -254,6 +254,10 @@ def get_path_metric_func(name): except KeyError as key: print("Path metric {0} not found. Valid selections: ".format(key))

Oliver Beckstein orbeckst@gmail.com * orbeckst@gmx.net

orbeckst · 2017-07-27T22:32:36Z

ping @sseyler

… test now checks KeyError is raised

kain88-de · 2017-08-01T14:28:01Z

testsuite/MDAnalysisTests/analysis/test_psa.py

@@ -171,17 +171,13 @@ class TestPSAExceptions(TestCase):
    def test_get_path_metric_func_bad_key(self):
        '''Test that KeyError is caught by
        get_path_metric_func().'''
-
-        try:
+        with self.assertRaises(KeyError):


with pytest.raises(KeyError): PSA.get_path_metric_func('foobar')

Yeah, I put my changes on hold since, once I rebased properly, realized everything is now pytest-specific. Hoping to finish this PR within the week.

kain88-de · 2017-08-01T14:30:45Z

package/MDAnalysis/analysis/psa.py

-    #     raise KeyError(err_str)
+        err_str = "Path metric {0} not found. Valid selections: ".format(key)
+        err_str += "  ".join(["\"{0}\"".format(name) for name in path_metrics.keys()])
+        raise KeyError(err_str)


You can use multiline strings

raise KeyError("Path metric {} not found. Valid Selections: {}".format( key, ' '.join(['"{}"'.format(name) for name in path_metrics.keys()]))

orbeckst · 2017-11-22T04:16:23Z

Ping Dr @sseyler , can you finish this up?

orbeckst · 2017-12-01T07:35:25Z

@sseyler said #1508 (comment):

Hoping to finish this PR within the week.

... on Aug 2. Time to finish it up before the year is over.

We should have the fixes for #1507 in 0.17.0 if possible (and the sooner we have higher test coverage the better!)

richardjgowers · 2017-12-10T21:31:34Z

Ok cool, this should be good to go. Make this a merge commit so that authorship isn't messed with

sseyler · 2017-12-20T21:33:36Z

@richardjgowers Forgot to say this earlier: Thanks, Richard! I was gonna have to do this over the holidays otherwise. I owe you a beer next time we meet!

sseyler closed this Jul 18, 2017

sseyler reopened this Jul 18, 2017

orbeckst added Component-Analysis testing labels Jul 18, 2017

sseyler added 8 commits July 19, 2017 12:47

Changed test name(s) from test_asymmetric_weight() to test_asymmetric…

b48275e

…_wavg_weight() and test_asymmetric_avg_weight(); need diff names for each test.

New names for (w)avg_hausdorff tests + reverting back to old (same) t…

46448c9

…est names: test_asymmetric_weight()

Added two tests for Hausdorff neighbors--only the distances

6bfbf1c

New test for Hausdorff pair frames in pairs analysis; pairs analysis …

3e3ac06

…now also gives distance matrix

Added fix, change, author sseyler to CHANGELOG; updated run_pairs_ana…

2ee4e65

…lysis docs for dist matrix D

Changed to old-style division in dist_mat_to_vec to ensure integer ou…

6b6fde1

…tput

Added docstring in PSAnalysis.cluster for dist_mat kwarg

7a5b9a7

sseyler force-pushed the issue-1006-psatests branch from e98f242 to 7a5b9a7 Compare July 19, 2017 19:55

richardjgowers reviewed Jul 20, 2017

View reviewed changes

get_path_metric_func now raises KeyError with error string; it's unit…

3f87c79

… test now checks KeyError is raised

kain88-de reviewed Aug 1, 2017

View reviewed changes

orbeckst added this to the 0.17.0 milestone Dec 1, 2017

Fixed up type checking in metric filename

37881f4

richardjgowers added 3 commits December 9, 2017 08:08

Updated error strings in analysis.psa

251df4d

Updated docstrings to numpy style in analysis.psa

b7af4d7

Merge branch 'develop' into issue-1006-psatests

3c75a00

richardjgowers approved these changes Dec 11, 2017

View reviewed changes

richardjgowers merged commit 8867646 into MDAnalysis:develop Dec 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1006: minor fixes and several new unit tests for PSA module #1508

Issue 1006: minor fixes and several new unit tests for PSA module #1508

sseyler commented Jul 18, 2017 •

edited

Loading

sseyler commented Jul 18, 2017

richardjgowers commented Jul 18, 2017

kain88-de commented Jul 18, 2017 •

edited

Loading

sseyler commented Jul 18, 2017

orbeckst commented Jul 18, 2017

orbeckst commented Jul 18, 2017

richardjgowers commented Jul 18, 2017

richardjgowers Jul 20, 2017

kain88-de Jul 21, 2017

richardjgowers Jul 20, 2017

orbeckst commented Jul 20, 2017 via email

orbeckst commented Jul 27, 2017

kain88-de Aug 1, 2017

sseyler Aug 2, 2017

kain88-de Aug 1, 2017

orbeckst commented Nov 22, 2017

orbeckst commented Dec 1, 2017

richardjgowers commented Dec 10, 2017

sseyler commented Dec 20, 2017

		@@ -254,6 +254,10 @@ def get_path_metric_func(name):
		except KeyError as key:
		print("Path metric {0} not found. Valid selections: ".format(key))

Issue 1006: minor fixes and several new unit tests for PSA module #1508

Issue 1006: minor fixes and several new unit tests for PSA module #1508

Conversation

sseyler commented Jul 18, 2017 • edited Loading

PR Checklist

sseyler commented Jul 18, 2017

richardjgowers commented Jul 18, 2017

kain88-de commented Jul 18, 2017 • edited Loading

sseyler commented Jul 18, 2017

orbeckst commented Jul 18, 2017

orbeckst commented Jul 18, 2017

richardjgowers commented Jul 18, 2017

richardjgowers Jul 20, 2017

Choose a reason for hiding this comment

kain88-de Jul 21, 2017

Choose a reason for hiding this comment

richardjgowers Jul 20, 2017

Choose a reason for hiding this comment

orbeckst commented Jul 20, 2017 via email

orbeckst commented Jul 27, 2017

kain88-de Aug 1, 2017

Choose a reason for hiding this comment

sseyler Aug 2, 2017

Choose a reason for hiding this comment

kain88-de Aug 1, 2017

Choose a reason for hiding this comment

orbeckst commented Nov 22, 2017

orbeckst commented Dec 1, 2017

richardjgowers commented Dec 10, 2017

sseyler commented Dec 20, 2017

sseyler commented Jul 18, 2017 •

edited

Loading

kain88-de commented Jul 18, 2017 •

edited

Loading