-
Notifications
You must be signed in to change notification settings - Fork 158
feat(lammps): load selected dump frames efficiently #1041
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,27 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import io | ||
| import os | ||
| import unittest | ||
|
|
||
| import numpy as np | ||
| from comp_sys import CompSys, IsPBC | ||
| from context import dpdata | ||
|
|
||
| from dpdata.formats.lammps import dump | ||
|
|
||
|
|
||
| class CountingStringIO(io.StringIO): | ||
| """Track how many lines the trajectory reader consumes.""" | ||
|
|
||
| def __init__(self, value): | ||
| super().__init__(value) | ||
| self.lines_read = 0 | ||
|
|
||
| def __next__(self): | ||
| self.lines_read += 1 | ||
| return super().__next__() | ||
|
|
||
|
|
||
| class TestLmpDumpSkip(unittest.TestCase, CompSys, IsPBC): | ||
| def setUp(self): | ||
|
|
@@ -20,3 +35,69 @@ def setUp(self): | |
| self.e_places = 6 | ||
| self.f_places = 6 | ||
| self.v_places = 4 | ||
|
|
||
|
|
||
| class TestLmpDumpFrameSelection(unittest.TestCase): | ||
| def setUp(self): | ||
| self.dump_file = os.path.join("poscars", "conf.5.dump") | ||
| self.type_map = ["O", "H"] | ||
|
|
||
| def test_select_frames_preserves_order_and_duplicates(self): | ||
| all_frames = dpdata.System( | ||
| self.dump_file, fmt="lammps/dump", type_map=self.type_map | ||
| ) | ||
| frame_indices = np.array([4, 1, 4]) | ||
|
|
||
| selected = dpdata.System( | ||
| self.dump_file, | ||
| fmt="lammps/dump", | ||
| type_map=self.type_map, | ||
| f_idx=frame_indices, | ||
| ) | ||
| expected = all_frames.sub_system(frame_indices) | ||
|
|
||
| np.testing.assert_allclose(selected["coords"], expected["coords"]) | ||
| np.testing.assert_allclose(selected["cells"], expected["cells"]) | ||
|
|
||
| def test_select_single_frame_by_integer(self): | ||
| selected = dpdata.System( | ||
| self.dump_file, | ||
| fmt="lammps/dump", | ||
| type_map=self.type_map, | ||
| f_idx=2, | ||
| ) | ||
| expected = dpdata.System( | ||
| self.dump_file, fmt="lammps/dump", type_map=self.type_map | ||
| )[2] | ||
|
|
||
| np.testing.assert_allclose(selected["coords"], expected["coords"]) | ||
| np.testing.assert_allclose(selected["cells"], expected["cells"]) | ||
|
|
||
| def test_file_object_is_read_once_and_stops_after_last_target(self): | ||
| with open(self.dump_file) as fp: | ||
| content = fp.read() | ||
| stream = CountingStringIO(content) | ||
|
|
||
| lines = dump.load_file(stream, f_idx=[1]) | ||
| frames = dump.split_traj(lines) | ||
|
|
||
| self.assertEqual(frames[0][1], "1") | ||
| self.assertLess(stream.lines_read, len(content.splitlines())) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion does not pin the behavior the PR is built to deliver. The file is 55 lines in 5 uniform 11-line frames, and More seriously,
The test name also promises "read once", but nothing in it verifies a single pass -- there is no seek/reopen counter. |
||
|
|
||
| def test_invalid_frame_indices(self): | ||
| with self.assertRaisesRegex(ValueError, "must not be empty"): | ||
| dump.load_file(self.dump_file, f_idx=[]) | ||
| with self.assertRaisesRegex(ValueError, "non-negative"): | ||
| dump.load_file(self.dump_file, f_idx=[-1]) | ||
| with self.assertRaisesRegex(IndexError, "out of range"): | ||
| dump.load_file(self.dump_file, f_idx=[5]) | ||
| with self.assertRaisesRegex(TypeError, "only integers"): | ||
| dump.load_file(self.dump_file, f_idx=["a"]) | ||
| with self.assertRaisesRegex(TypeError, "only integers"): | ||
| dump.load_file(self.dump_file, f_idx=[True]) | ||
| with self.assertRaisesRegex(TypeError, "an iterable of integers"): | ||
| dump.load_file(self.dump_file, f_idx=1.5) | ||
|
|
||
| def test_frame_indices_are_mutually_exclusive_with_slice(self): | ||
| with self.assertRaisesRegex(ValueError, "cannot be combined"): | ||
| dump.load_file(self.dump_file, begin=1, f_idx=[2]) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a regression on the default (no-
f_idx) path.The old loader's
line = fp.readline().rstrip("\n")/if not line: return lineswas doing double duty -- EOF test and blank-line terminator (since"\n".rstrip("\n") == ""), going back to a8edc0b. Iteratingfor raw_line in fpkeeps only the EOF half, so a blank line now gets appended into the frame via thiselif frame:branch and eventually reachesget_atype'sint(ii.split()[id_idx]).With
tests/poscars/conf.5.dumpplus one trailing newline: master loads 5 frames, this branch raisesIndexError: list index out of range. A blank line between frames does the same (master silently returned 1 frame). Trailing newlines are common in concatenated or post-processed dumps, and no fixture intests/has one, so CI cannot catch it.Suggest
if not line: continueafter the TIMESTEP check.Note the same input also breaks one layer down at
split_traj(line 447), where the final block now ends atlen(dump_lines)instead of being clamped --load_file(src) + ["# end of run"]givesValueError: invalid literal for int() with base 10: '#'. Worth fixing both, plus a test with a trailing blank line.(Unrelated to this, the
split_trajrewrite to real TIMESTEP boundaries is a good change -- please keep it.)