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

[Review Needed]Pytest Style TestEncore #1609

Merged
merged 8 commits into from Sep 3, 2017

Conversation

Projects
None yet
2 participants
@utkbansal
Member

utkbansal commented Aug 8, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

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

utkbansal added some commits Aug 8, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 11, 2017

result_value = results[0, 1]
min_bound = 1E5
self.assertGreater(result_value, min_bound,
msg="Unexpected value for Harmonic Ensemble Similarity: {0:f}. Expected {1:f}.".format(result_value, min_bound))
assert result_value >= min_bound, "Unexpected value for Harmonic " \

This comment has been minimized.

@kain88-de

kain88-de Aug 13, 2017

Member

this is equivalent with self.assertGreaterEqual and not a pure greater cheak

result_value = results[0,1]
upper_bound = 0.6
self.assertLess(result_value, upper_bound,
msg="Unexpected value for Dim. reduction Ensemble Similarity: {0:f}. Expected {1:f}.".format(result_value, upper_bound))
assert result_value <= upper_bound, "Unexpected value for Dim. " \

This comment has been minimized.

@kain88-de

kain88-de Aug 13, 2017

Member

this is not a self.assertLess remove the equal sign.

np.copy(cls.ens2_template.trajectory.timeseries(format='fac')[::5, :, :]),
assert average <= average_upper_bound, "Unexpected average value for " \
"bootstrapped samples in Dim. reduction Ensemble similarity"
assert stdev <= stdev_upper_bound, "Unexpected standard deviation for" \

This comment has been minimized.

@kain88-de

kain88-de Aug 13, 2017

Member

see comment above for both asserts

distances += YY
np.maximum(distances, 0, out=distances)
distances.flat[::distances.shape[0] + 1] = 0.0
dimension = len(distances)

This comment has been minimized.

@kain88-de

kain88-de Aug 13, 2017

Member

these should be in a fixture since arrays are not necessarily constant.

template = mda.Universe(
template.filename,
np.copy(template.trajectory.timeseries(format='fac')[::5, :, :]),
format=mda.coordinates.memory.MemoryReader)

This comment has been minimized.

@kain88-de

kain88-de Aug 17, 2017

Member

I think transfer_to_memory has a step argument now that you can use instead of this trick.

This comment has been minimized.

@kain88-de

kain88-de Aug 27, 2017

Member

Why do you still not use the step argument of transfer_to_memory here?

distances += XX
distances += YY
np.maximum(distances, 0, out=distances)
distances.flat[::distances.shape[0] + 1] = 0.0

This comment has been minimized.

@kain88-de

kain88-de Aug 17, 2017

Member

all this setup code should be in a fixture for distance_matrix. It's he only one that uses this. I also don't see the use for the fixture of dimensions

@utkbansal utkbansal changed the title from [WIP]Pytest Style TestEncore to [Review Needed]Pytest Style TestEncore Aug 24, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 26, 2017

@kain88-de I've used transfer_to_memory. Anything else needed here?

template = mda.Universe(
template.filename,
np.copy(template.trajectory.timeseries(format='fac')[::5, :, :]),
format=mda.coordinates.memory.MemoryReader)

This comment has been minimized.

@kain88-de

kain88-de Aug 27, 2017

Member

Why do you still not use the step argument of transfer_to_memory here?

template.transfer_to_memory(step=5)
template = mda.Universe(
template.filename,
np.copy(template.trajectory.timeseries(format='fac')),

This comment has been minimized.

@kain88-de

kain88-de Aug 27, 2017

Member

Do you know what this statement is doing? After using transfer to memory this shouldn't be necessary anymore.

This comment has been minimized.

@utkbansal

utkbansal Sep 1, 2017

Member

I don't think I have a real understanding of that statement.

@kain88-de kain88-de merged commit 49aaf08 into MDAnalysis:develop Sep 3, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.006%) to 90.583%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment