feat(lammps): load selected dump frames efficiently - #1041
Conversation
Add f_idx-based sparse frame loading for LAMMPS dump trajectories while preserving requested order and duplicate indices. Stop scanning after the final requested frame and validate invalid selections explicitly. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Merging this PR will not alter performance
|
dosubot
Bot
added
size:L
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLAMMPS dump loading now supports selecting specific frame indices, including ordered duplicates, with input validation and early termination. The plugin forwards ChangesLAMMPS frame selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant LAMMPSDumpFormat
participant load_file
participant DumpStream
Caller->>LAMMPSDumpFormat: request frames with f_idx
LAMMPSDumpFormat->>load_file: forward f_idx
load_file->>DumpStream: iterate complete dump frames
DumpStream-->>load_file: yield frames through last requested index
load_file-->>LAMMPSDumpFormat: return ordered selected lines
LAMMPSDumpFormat-->>Caller: construct selected system
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_lammps_dump_skipload.py (1)
87-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a case for the
TypeErrorpath.Coverage here checks empty/negative/out-of-range but not
f_idxcontaining a non-integer element (e.g.f_idx=["a"]), which_normalize_frame_indicesis supposed to reject withTypeError.✅ Suggested addition
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"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lammps_dump_skipload.py` around lines 87 - 93, Add a TypeError assertion to test_invalid_frame_indices for a non-integer frame index such as f_idx=["a"], matching the rejection behavior of _normalize_frame_indices and checking the expected error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_lammps_dump_skipload.py`:
- Around line 87-93: Add a TypeError assertion to test_invalid_frame_indices for
a non-integer frame index such as f_idx=["a"], matching the rejection behavior
of _normalize_frame_indices and checking the expected error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04b3650b-88e9-4513-a996-777a60b2d15e
📒 Files selected for processing (3)
dpdata/formats/lammps/dump.pydpdata/plugins/lammps.pytests/test_lammps_dump_skipload.py
Add the non-integer element case suggested in review, plus the bool rejection and the non-iterable scalar case, so both `TypeError` branches of `_normalize_frame_indices` are exercised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1041 +/- ##
==========================================
+ Coverage 87.63% 87.66% +0.02%
==========================================
Files 90 90
Lines 9209 9247 +38
==========================================
+ Hits 8070 8106 +36
- Misses 1139 1141 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Addressed the review nitpick in b68b96d: |
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
The f_idx implementation itself is well done -- order/duplicate preservation, early termination, and the atom-id remap under selection all check out, and I confirmed System(f, f_idx=[1,2]) matches System(f).sub_system([1,2]) exactly. Two things I'd like addressed before merge. Note that CodeRabbit was rate limited on this PR and never actually reviewed it.
| if frame: | ||
| yield frame | ||
| frame = [line] | ||
| elif frame: |
There was a problem hiding this comment.
This is a regression on the default (no-f_idx) path.
The old loader's line = fp.readline().rstrip("\n") / if not line: return lines was doing double duty -- EOF test and blank-line terminator (since "\n".rstrip("\n") == ""), going back to a8edc0b. Iterating for raw_line in fp keeps only the EOF half, so a blank line now gets appended into the frame via this elif frame: branch and eventually reaches get_atype's int(ii.split()[id_idx]).
With tests/poscars/conf.5.dump plus one trailing newline: master loads 5 frames, this branch raises IndexError: 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 in tests/ has one, so CI cannot catch it.
Suggest if not line: continue after the TIMESTEP check.
Note the same input also breaks one layer down at split_traj (line 447), where the final block now ends at len(dump_lines) instead of being clamped -- load_file(src) + ["# end of run"] gives ValueError: invalid literal for int() with base 10: '#'. Worth fixing both, plus a test with a trailing blank line.
(Unrelated to this, the split_traj rewrite to real TIMESTEP boundaries is a good change -- please keep it.)
| frames = dump.split_traj(lines) | ||
|
|
||
| self.assertEqual(frames[0][1], "1") | ||
| self.assertLess(stream.lines_read, len(content.splitlines())) |
There was a problem hiding this comment.
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 f_idx=[1] reads 23 -- but the assertion only requires < 55, so an implementation that stopped two frames late (45 lines) passes identically.
More seriously, CountingStringIO only counts __next__, so a readline()-based loader that reads the whole file with no early termination at all records lines_read = 0 and passes this test. I confirmed that by writing one. Since this is the only test backing the 14x speedup claim, a regression to the pre-PR read strategy would go undetected.
assertLessEqual(stream.lines_read, 23) would pin it; tracking the maximum stream position across read/readline/readlines (or asserting stream.tell() < len(content)) would close the gap properly. This is the same point CodeRabbit raised on the closed #1040, which this probe was copied from.
The test name also promises "read once", but nothing in it verifies a single pass -- there is no seek/reopen counter.
3 participants
Summary
f_idxsupport tolammps/dumpso callers can load arbitrary non-negative frame indices directlySystem.sub_systembegin/stepselectionsITEM: TIMESTEPboundaries instead of assuming a fixed block lengthExample:
Performance benchmark
The benchmark compares direct sparse loading against the existing workflow of loading the complete trajectory and then calling
sub_system(indices).System(..., f_idx=indices)System(...).sub_system(indices)Direct sparse loading was 14.38x faster and reduced peak RSS by 64.39% for this workload. Both methods returned the same 10 selected frames. Because the final frame was selected, both methods traversed the full file; the improvement comes from avoiding storage and parsing of unselected frames.
Validation
python -m unittest test_lammps_dump_skipload.py test_lammps_dump_to_system.py test_lammps_dump_unfold.py test_lammps_dump_shift_origin.py test_lammps_dump_idx.py test_lammps_read_from_trajs.py test_lammps_spin.py— 70 tests passedruff check dpdata/ tests/test_lammps_dump_skipload.py— passedruff format --check dpdata/ tests/test_lammps_dump_skipload.py— passeddpdata --helpanddpdata --version— passedparmeddependency is not installed in the environment, and 43 tests were skippedCloses #367.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
New Features
Bug Fixes