DL_POLY HISTORY may contain cells even for non-periodic simulations #3312
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3312 +/- ##
===========================================
- Coverage 93.65% 89.60% -4.05%
===========================================
Files 176 167 -9
Lines 22841 21417 -1424
Branches 3197 0 -3197
===========================================
- Hits 21391 19191 -2200
- Misses 1399 2226 +827
+ Partials 51 0 -51
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hey @fiszczyp thanks for the fix. To include this we'll also need a test written, you might be able to alter one of the existing test files for this.
|
Hello @fiszczyp! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-05-22 15:55:18 UTC |
| while not len(line.split()) == 5: | ||
| line = inf.readline() |
There was a problem hiding this comment.
I think this has the ability to infinitely loop if a (non HISTORY) file never has 5 tokens on a line. I think once we reach end-of-file readline() is going to return an empty string (i.e. not even a newline char), can we add a check for that and raise an error if we reach the end of file?
There was a problem hiding this comment.
It now raises EOFError, thanks!
| # Second line is traj_key, imcom, n_atoms, n_frames, n_records | ||
| offsets = [] | ||
|
|
||
| with open(self.filename, 'r') as f: |
There was a problem hiding this comment.
The reason for self._n_frames was to cache the number of frames so it's only calculated once. This needs to be added back in.
There was a problem hiding this comment.
Actually, a question about the expected behaviour if a wrong HISTORY file is passed. DL_POLY HISTORY files have the n_frames added to the header line on top, so from the test files DLP_HISTORY_minimal:
DL_POLY: Minimal example of HISTORY
0 0 3 3 2606
it can be parsed as:
trajectory key: 0 - only coordinates saved
imcom: 0 - non periodic
n_atoms: 3 - three atoms
n_frames: 3 - three frames
n_records: 2606 - number of records (typically lines, wrong here).
The HistoryReader() takes n_atoms from there. I also made it take n_frames from there, but it might be that someone just truncated the trajectory file and the information might be wrong. Should we instead just go through the file updating the number of frames as it reads? That is:
n_frames = len(offsets).
I think it might be more bullet proof, as I can totally imagine when someone would remove some last/first 50 frames without updating a header, but not touch the number of atoms at all.
There was a problem hiding this comment.
Re caching, I don't think the HistoryReader() was doing that but it makes sense to add, thanks!
|
thanks @fiszczyp can you also add yourself to AUTHORS and we can merge this |
Done, thanks! |
Fixes #3314
DL_POLY HISTORY files can still have cell, even if the boundary conditions are switched off (imcom=0).
Changes made in this Pull Request:
_has_cell(akin to_has_velocity, etc.) to use as a flag instead ofnot imcom == 0for DL_POLY trajectory HistoryReader.if not all(...)toif not np.all(...)as I think it is deprecated and was throwing errors.not imcom == 0check there and instead just checked once the line length corresponds to a correct atom entry to skip all the cell parameters.PR Checklist