-
Notifications
You must be signed in to change notification settings - Fork 643
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
Change frame numbering from 1-based to 0-based #310
Comments
I've started this process on issue-310. The general strategy is:
|
I need help from someone familiar with the DCD pyrex code. I'm having trouble parsing it, so if someone wants to push a commit to this branch with the needed fixes to indexing there I'd be grateful. |
some changes to Reader's iters just got pushed so make sure you're working with them, it should make your life easier too. Something that might trip you up, I've seen a lot of Readers using the fact that frame is 1 based to know the index of the next frame ( in 0 based)... I think PDB was the culprit, but then this was used as a template for many others. |
Thanks for the tip. I've also noticed that many of them initialize |
So if everything is passing, all that needs to be done is making sure that the tests for each Reader are actually checking that:
The testing for coordinate Readers is pretty haphazard currently, there should really be some sort of overarching |
@orbeckst I believe the relevant code is in |
Hey @dotsdl I pushed what is probably the the commit I'm least proud of ever, but it should make DCD work! The tests work before and after my hack (I checked it manually, gives 0 based now) I'm getting a fail in test_analysis... If you want I can write the tests I proposed above |
Running the tests now. I just pushed the changes needed to make the current set of tests meaningful for 0-based DCD frame reading. It looks like this hack works; I've got no better solutions, even after poking the pyrex code for a little while now. |
I'm about to start checking each reader test to ensure they do those tests you propose. If you want to check, too, I appreciate the help. You're welcome to start with the DCD reader. :D |
FYI: DCD.c is pure C, no cython/pyrex. The Python objects are hand-coded. |
We don't need to change it all, at least not with the fix @richardjgowers introduced, where we just decrement the frame number each time a frame is read. |
Ok, good. Sorry, didn't have time to dig into the commit. On 18 Jun, 2015, at 14:43, David Dotson wrote:
|
Okay, we now have 0-based frames for all readers except for the TRZ reader. Do TRZs store a frame number internally? I can't figure out where the tests are getting these frame numbers for that reader. |
@dotsdl Yeah the frame is stored in the trajectory, fixed now. |
@richardjgowers awesome. We're almost there! I just realized I mispoke: the mol2 reader doesn't appear to update frame information at all on iteration. I noticed this in 610e300 but failed to mention it in my last post. I'm pulling my hair out trying to figure out why this is the case. Ideas? |
Hehe, this one was pretty funny. MOL2 was creating a new Timestep object in each read, so was returning a Timestep with all the correct info. The problem was the tests were checking against the original Timestep (which wasn't touched in the read). So you were holding a pointer to a Timestep that MOL2 wasn't using any more.. I've fixed this so that MOL2 reuses the same Timestep throughout (in line with other Readers) and tests now pass |
It might be something worth writing a test for... something like ts = u.trajectory.ts
ts2 = u.trajectory.next()
assert ts is ts2 |
Okay...there are just a few tests that are failing yet. One has to do with streamio on the XYZReader, which I don't know how to fix. There are also failures from assert_equal(l, 0) back to assert_equal(l, 1) results in the value of |
In other words, for assert_equal(l, 0) we get
But with: assert_equal(l, 1) we get
What is going on here?! |
For those tests, I wrote 2 "reader" classes at the top of the test, these will have to be changed to give 0 based frame numbers. |
Ok I've updated the Reader API tests to now expect and use 0 based frames throughout |
Awesome. The only test we need to fix now is the |
I think I've squashed it now. Reading numframes was messing with the file handle |
Great! Running all tests now. If they all pass, mind if I squash, apply any missing |
Yeah if you could that would be awesome. I'm keen to get this through so I can finish #239 |
Hmmm ok, so the fix I wrote works in 2.7, but not 2.6, because the bz2 doesn't support context management in 2.6? I'll rewrite the fix, but it shouldn't affect all the fun you're having writing docs |
Heh. :D |
*Special thanks to @richardjgowers for lots of work on troublesome Readers.* Went through each Reader class; changed frame to 0-based. Fixed tests relating to 0 based frame numbering Added 0 based frames to DCD Modified tests for DCDs. Added 0-based frame tests to DMS, GMS test classes. Added new tests for MOL2 reader checking frame indexing. Finished tests for XYZ reader frames indices. Fixed TRZ Reader frame numbering. TRZReader now reports Time from Timestep not based on dt * frame. Fixed MOL2 reader 0-based numbering. Now reuses Timesteps (like other Readers) Now has _reopen method which "rewinds" the frame numbering The helanal analysis could use a redesign, but in the meantime fixed to ensure that it works as expected regarding trajectory times. Fixed test_reader_api tests to work with 0-based frames. Updated Reader API checks for 0 based operation Fixed XYZReader not rewinding properly on checking numframes Added final .. versionchanged directives to classes, module docs
*Special thanks to @richardjgowers for lots of work on troublesome Readers.* Went through each Reader class; changed frame to 0-based. Fixed tests relating to 0 based frame numbering Added 0 based frames to DCD Modified tests for DCDs. Added 0-based frame tests to DMS, GMS test classes. Added new tests for MOL2 reader checking frame indexing. Finished tests for XYZ reader frames indices. Fixed TRZ Reader frame numbering. TRZReader now reports Time from Timestep not based on dt * frame. Fixed MOL2 reader 0-based numbering. Now reuses Timesteps (like other Readers) Now has _reopen method which "rewinds" the frame numbering The helanal analysis could use a redesign, but in the meantime fixed to ensure that it works as expected regarding trajectory times. Fixed test_reader_api tests to work with 0-based frames. Updated Reader API checks for 0 based operation Fixed XYZReader not rewinding properly on checking numframes Added final .. versionchanged directives to classes, module docs
So as not to disrupt your work, I've squashed the commits for this issue and pushed them to issue-310-fin. If you push your final bits to the original |
*Special thanks to @richardjgowers for lots of work on troublesome Readers.* Went through each Reader class; changed frame to 0-based. Fixed tests relating to 0 based frame numbering Added 0 based frames to DCD Modified tests for DCDs. Added 0-based frame tests to DMS, GMS test classes. Added new tests for MOL2 reader checking frame indexing. Finished tests for XYZ reader frames indices. Fixed TRZ Reader frame numbering. TRZReader now reports Time from Timestep not based on dt * frame. Fixed MOL2 reader 0-based numbering. Now reuses Timesteps (like other Readers) Now has _reopen method which "rewinds" the frame numbering The helanal analysis could use a redesign, but in the meantime fixed to ensure that it works as expected regarding trajectory times. Fixed test_reader_api tests to work with 0-based frames. Updated Reader API checks for 0 based operation Fixed XYZReader not rewinding properly on checking numframes Added final .. versionchanged directives to classes, module docs Fixed 2.6 XYZReader
*Special thanks to @richardjgowers for lots of work on troublesome Readers.* Went through each Reader class; changed frame to 0-based. Fixed tests relating to 0 based frame numbering Added 0 based frames to DCD Modified tests for DCDs. Added 0-based frame tests to DMS, GMS test classes. Added new tests for MOL2 reader checking frame indexing. Finished tests for XYZ reader frames indices. Fixed TRZ Reader frame numbering. TRZReader now reports Time from Timestep not based on dt * frame. Fixed MOL2 reader 0-based numbering. Now reuses Timesteps (like other Readers) Now has _reopen method which "rewinds" the frame numbering The helanal analysis could use a redesign, but in the meantime fixed to ensure that it works as expected regarding trajectory times. Fixed test_reader_api tests to work with 0-based frames. Updated Reader API checks for 0 based operation Fixed XYZReader not rewinding properly on checking numframes Added final .. versionchanged directives to classes, module docs Fixed 2.6 XYZReader
Please check all the docs and update accordingly, e.g. http://www.mdanalysis.org/mdanalysis/package/doc/html/documentation_pages/coordinates/init.html#id13 |
All right. I'm back in business. I'll take care of this. |
@dotsdl , is there anything else to be done here than the docs? If not then this should be an easy "close"... |
Just the docs. Need to go through with a comb. Doing this today in addition to #309. |
Closed in 5bdbb47 |
Subtask for #252:
We want to make frame numbering start from 0 so that indexing with, say:
gives frame 0 instead of frame 1. This change will likely break lots of code that includes
ts.frame + 1
andts.frame - 1
and the like; will need to go through the code with a fine-toothed comb for these instances, using the tests as markers for trouble.The text was updated successfully, but these errors were encountered: