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.

Sign up for GitHub

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

Jump to bottom

Fix and cover RMSD #889

Merged
merged 6 commits into from
Jul 10, 2016
Merged

Fix and cover RMSD #889

merged 6 commits into from
Jul 10, 2016

Conversation

jdetle
Copy link
Contributor

@jdetle jdetle commented Jul 6, 2016 •
edited
Loading

Fixes: #897

Changes made in this Pull Request:

  • Fixed a bug where we are still calling _process_selection as opposed to process_selection in RMSD (one that I think I created a while back)
    • Changed RMSD to save numpy array.
    • Added rmsdArray testfile
    • Added RMSD tests

TODO

  • Cover exceptions in RMSD
  • Cover logger warnings
  • Revert to savetxt
  • Change to psf, dcd, start, stop, step to speed up tests
  • Figure out how to load txt files as np arrays to compare, or some other way to test save function
  • Test when reference not none in rmsd
  • Assert selection error when reference not equal in length to traj atoms
  • Same for group selections
  • Test mass_weighted in run
  • Rename rmsd tests
  • Undo update of changelog

Currently left for another PR

  • Cover process_selection errors
  • Figure out how to force an overflow underflow error in RMSF

Currently stuck

  • Can open file handles be closed, if so, how?
  • Should tests always assert something? For example, I have a test to cover saving and weighting, but I don't actually check if the result is equal to some expected value.

@coveralls
Copy link

coveralls commented Jul 6, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 80.38% when pulling 18a4e4c on jdetle:process_selection into 7220db5 on MDAnalysis:develop.

@jdetle jdetle force-pushed the process_selection branch from 18a4e4c to c868ed2 Compare July 6, 2016 22:54
@coveralls
Copy link

coveralls commented Jul 6, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 80.364% when pulling c868ed2 on jdetle:process_selection into 852daec on MDAnalysis:develop.

@kain88-de
Copy link
Member

why wasn't this raising an error in the tests? Isn't this covered in the test?

@jdetle
Copy link
Contributor Author

jdetle commented Jul 7, 2016

This coverage report seems to indicate not! . I will go ahead and write some tests for the RMSD class.

@jdetle
Copy link
Contributor Author

jdetle commented Jul 7, 2016

It seems like RMSD is ripe for a refactor to the Bauhaus API too, so I could just go ahead and do that as well.

@jdetle jdetle mentioned this pull request Jul 7, 2016
Closed
@kain88-de
Copy link
Member

Yes if you like. Please finish the dmap first though

@jdetle jdetle changed the title Removed call to protected process_selection now that it isn't protected [WIP] Fix RMSD, (which I broke) and add tests Jul 8, 2016
@jdetle jdetle changed the title [WIP] Fix RMSD, (which I broke) and add tests [WIP] Fix and cover RMSD, (which I broke) Jul 8, 2016
@jdetle jdetle changed the title [WIP] Fix and cover RMSD, (which I broke) [WIP] Fix and cover RMSD Jul 8, 2016
@orbeckst
Copy link
Member

orbeckst commented Jul 8, 2016

Leave refactor for later (as Max said). Fixing the bug and some tests are good enough for this PR.

Re: naming: I don't like RMSDSingleFrame, it still looks like a class. But ultimately your choice, just do it :-).

@jdetle
Copy link
Contributor Author

jdetle commented Jul 8, 2016

On it.

@jdetle jdetle force-pushed the process_selection branch from 1ea25b1 to 856b56b Compare July 8, 2016 22:10
@orbeckst orbeckst added this to the 0.16.0 milestone Jul 8, 2016
@orbeckst orbeckst self-assigned this Jul 8, 2016
@jdetle jdetle force-pushed the process_selection branch from 856b56b to f051b47 Compare July 8, 2016 23:09
@coveralls
Copy link

coveralls commented Jul 8, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.4%) to 80.774% when pulling f051b47 on jdetle:process_selection into 5d78f03 on MDAnalysis:develop.

@jdetle
Copy link
Contributor Author

jdetle commented Jul 9, 2016

Now is as good as a time as any, what are open file handles and how do I close them?

@coveralls
Copy link

coveralls commented Jul 9, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.4%) to 80.805% when pulling 8b3d00a on jdetle:process_selection into 5d78f03 on MDAnalysis:develop.

@jdetle jdetle changed the title [WIP] Fix and cover RMSD Fix and cover RMSD Jul 9, 2016
@jdetle jdetle changed the title Fix and cover RMSD [WIP] Fix and cover RMSD Jul 9, 2016
orbeckst
orbeckst reviewed Jul 9, 2016
View reviewed changes
package/CHANGELOG
@@ -25,6 +25,7 @@ Fixes
next (Issue #869)
* Display of Deprecation warnings doesn't affect other modules anymore (Issue #754)
* Changed nframes to n_frames in analysis modules for consistency (Issue #890)
* Removed call to protected process_selection function in RMSD
Copy link
Member

Choose a reason for hiding this comment

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

Leave this out from the CHANGELOG. I don't think that this was broeken in 0.15.0 or was it? If it was broken then this would be an entry under fixes, just stating that rms.RMSD was fixed (and reference #897)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Thanks.

@coveralls
Copy link

coveralls commented Jul 9, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.5%) to 80.897% when pulling 0358b44 on jdetle:process_selection into 5d78f03 on MDAnalysis:develop.

orbeckst
orbeckst reviewed Jul 9, 2016
View reviewed changes
package/MDAnalysis/analysis/rms.py
@@ -527,7 +527,7 @@ def save(self, filename=None):
if filename is not None:
if self.rmsd is None:
raise NoDataError("rmsd has not been calculated yet")
np.savetxt(filename, self.rmsd)
np.save(filename, self.rmsd)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this? This can break code for users, don't do this light-heartedly! Please revert!

save and savetxt are two very different things.

@jdetle
Copy link
Contributor Author

jdetle commented Jul 9, 2016 •
edited
Loading

There are a few mistakes and additional tests that I will go ahead and work on tonight. Beyond that, packing for a weekend in the bay before _SCIPY 2016_

@jdetle jdetle force-pushed the process_selection branch from e50390c to 023408d Compare July 9, 2016 06:15
@coveralls
Copy link

coveralls commented Jul 9, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 81.013% when pulling 023408d on jdetle:process_selection into 5d78f03 on MDAnalysis:develop.

@jdetle
Copy link
Contributor Author

jdetle commented Jul 9, 2016

Spurious test failures

@jdetle jdetle changed the title [WIP] Fix and cover RMSD Fix and cover RMSD Jul 9, 2016
@orbeckst
Copy link
Member

orbeckst commented Jul 9, 2016

Rerun the test on Travis. There should be a little reload button in the upper right. At least there is one for me when I have to do this.

Oliver Beckstein
email: [email protected]

Am Jul 8, 2016 um 23:59 schrieb John Detlefs [email protected]:

Spurious test failures

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

@jdetle
Copy link
Contributor Author

jdetle commented Jul 9, 2016

I have it but for some reason it throws an error saying the job can't be restarted, it's either a bug or a permissions issue.

@jdetle
Copy link
Contributor Author

jdetle commented Jul 9, 2016

I will be away from the computer until late tonight. I hope everything looks alright. 1% coverage increase has to be nice :)

@orbeckst
Copy link
Member

orbeckst commented Jul 9, 2016

Travis appears to be happy now. Everything passed.

@orbeckst
Copy link
Member

orbeckst commented Jul 9, 2016

Still WIP or ready to be merged?

@jdetle
Copy link
Contributor Author

jdetle commented Jul 9, 2016

Ready to be merged. All the todos are todone. There are still some minor
questions that I have about testing but merge-able nonetheless.
On Sat, Jul 9, 2016 at 3:01 PM Oliver Beckstein [email protected]
wrote:

Still WIP or ready to be merged?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#889 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AKcARg7JHi8iW2fNW77Hlq6usB7RvXrVks5qUBo_gaJpZM4JGYJ2
.

Have a wonderful day,
John J. Detlefs

@orbeckst
Copy link
Member

orbeckst commented Jul 9, 2016

Ah, just read the top post where you said:

  1. Can open file handles be closed, if so, how?
  2. Should tests always assert something? For example, I have a test to cover saving and weighting, but I don't actually check if the result is equal to some expected value

Answers:

  1. Good question, @jbarnoud 's plugin reports files that are still open but I also don't have a clue how to fix these reported open files.
  2. Each test should always assert something. At a minimum, check that a file was written, but better (and that's what we do in the trajectory writer tests), read the file back in and compare to the data that you used to write the file. If that is not feasible, manually read the file, compute some value (eg a mean) and check that you get it back from the saved file in the test.

orbeckst
orbeckst reviewed Jul 9, 2016
View reviewed changes
package/MDAnalysis/analysis/rms.py
@@ -83,7 +83,7 @@

import matplotlib.pyplot as plt
rmsd = R.rmsd.T # transpose makes it easier for plotting
time = rmsd[1]
time = rmsd[1]facebook.com
Copy link
Member

Choose a reason for hiding this comment

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

There's a random "facebook.com" in the docs. Please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird...

@orbeckst
Copy link
Member

orbeckst commented Jul 9, 2016

@jdetle very good job! I'd like you to attend to the comments – primarily always assert something.

Thanks a lot for fixing and testing RMSD.

@jdetle
Copy link
Contributor Author

jdetle commented Jul 10, 2016

Comments attended to!

@coveralls
Copy link

coveralls commented Jul 10, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 81.015% when pulling 91ba82a on jdetle:process_selection into 5d78f03 on MDAnalysis:develop.

@orbeckst orbeckst merged commit e801d35 into MDAnalysis:develop Jul 10, 2016
@orbeckst
Copy link
Member

Thanks a lot @jdetle, good work!

@jdetle jdetle mentioned this pull request Jul 11, 2016
Merged
13 tasks
abiedermann pushed a commit to abiedermann/mdanalysis that referenced this pull request Oct 26, 2016
@jdetle @orbeckst
Fix and cover RMSD (MDAnalysis#889)
8dcb4ad 
Fixed (temporarily) broken analysis.rms.RMSD and added missing tests

- fixes MDAnalysis#897 
- full testing of the RMSD class

Updated CHANGELOG

* Added tests for RMSD, added a testfile.
Changed RMSD class to save rmsd array as numpy file instead of text.

* Tested saving

* Test group selections
Reverted back to save.
Updated test coverage for exceptions.
Found some errors with logging requiring a message.
Reverted inclusion of unnecessary RMSD array.
Fixed a small error and combined save and mass_weighted test.
No Data Error.
Covered RMSD.save().

* Fixed error messages

* Asserted everywhere, fixed error messages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@orbeckst orbeckst

Labels
Component-Analysis defect
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.

analysis.rms.RMSD broken
4 participants
@jdetle @coveralls @kain88-de @orbeckst