Conversation
|
Hello @yuxuanzhuang! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-08-16 14:55:01 UTC |
Codecov Report
@@ Coverage Diff @@
## develop #2911 +/- ##
===========================================
- Coverage 92.95% 92.94% -0.02%
===========================================
Files 187 187
Lines 24579 24591 +12
Branches 3185 3185
===========================================
+ Hits 22847 22855 +8
- Misses 1686 1690 +4
Partials 46 46
Continue to review full report at Codecov.
|
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Good that we had a look at this again. I think this looks right but please see comments. If it's the way that I think it is then we need to add a bit of documentation to the code and at least the DCDReader.
However, I am not really keen on making DCDFile.seek() behave differently from any other seek. Could we do the following: Make the second part of DCDFile.seek() starting at self.reached_eof = False a private method _seek() that only checks that its input frame is 0 <= frame <= self.n_frames. DCDFile.__setstate__ can call _seek() but DCDFile.seek() would check 0 <= frame < self.n_frames before calling _seek() and so continue to behave like all the other Reader.seek()` methods.
|
|
||
| """ | ||
| if frame >= self.n_frames: | ||
| if frame >= self.n_frames + 1: |
There was a problem hiding this comment.
Is this supposed to allow to seek to the end of the file? If so, add a comment, because under normal circumstances I expect a 0-based index for a sequence to fail when it hits len(sequence).
There was a problem hiding this comment.
If it is possible to seek to the end of the file (after the last coordinate frame) with
dcd.seek(len(dcd))then state it in the doc string. This is a fairly odd thing to allow so it should be documented.
| if current_frame == self.offsets.size: | ||
| # cannot seek to self.offsets.size (like in DCD file) | ||
| # because here seeking depends on the offsets list size. | ||
| # Instead, we seek to one frame ahead and read the next frame. |
There was a problem hiding this comment.
Did you mean "previous frame" instead of "frame ahead"?
| current_frame = state[1] | ||
| self.seek(current_frame - 1) | ||
| self.current_frame = current_frame | ||
| if current_frame == self.offsets.size: |
There was a problem hiding this comment.
Switch the decision in the if statement to make the more common situation come first
if current_frame < self.offsets.size:
...
elif current_frame == self.offsets.size:
...
else:
#pragma: no cov
raise RuntimeError("Invalid frame number {} > {} -- this should not happen.".format(current_frame, self.offsets.size)and add defensive code.
There was a problem hiding this comment.
Add comment under which circumstances we have to do funny things – namely when we need to serialize at the end of the trajectory.
|
I makes |
|
The |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
This looks like a good solution, just doing the special casing inside __setstate__.
It's ok that "no cover" didn't work, thanks for finding out why.
- Fixes MDAnalysis#2878 - Changes made in this Pull Request: - add more comprehensive tests for pickling low-level dcd and xdr files - fix a bug that was introduced in MDAnalysis#2723 (wrong seeking in DCD and XDR, note: this bug was only in develop, never in released code, see discussion in PR MDAnalysis#2908) - update CHANGELOG - maybe backport in MDAnalysis#2908
4 participants
Fixes #2878
Changes made in this Pull Request:
PR Checklist