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
Changes from 2 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