fix(system): copy first append and support absolute paths#1015
fix(system): copy first append and support absolute paths#1015njzjz-bot wants to merge 2 commits into
Conversation
Deep-copy data when appending into an empty system and build MultiSystems.from_dir glob patterns without a forced relative prefix. Regression tests cover source ownership and equivalent relative/absolute directory searches. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
dosubot
Bot
added
size:XS
📝 WalkthroughWalkthrough
ChangesSystem data isolation
Absolute directory search
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/test_multisystems.py`:
- Around line 54-64: Strengthen
test_absolute_directory_matches_relative_directory in TestMultiSystemsFromDir to
compare the matched systems themselves, not just collection and aggregate frame
counts. Assert equivalent system names and per-system frame counts, or reuse the
repository’s existing system-comparison helper, while preserving the
relative-versus-absolute directory setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24696385-2be8-420e-aabb-8f15318c1c0d
📒 Files selected for processing (2)
dpdata/system.pytests/test_multisystems.py
| class TestMultiSystemsFromDir(unittest.TestCase): | ||
| def test_absolute_directory_matches_relative_directory(self): | ||
| relative = dpdata.MultiSystems.from_dir( | ||
| "poscars", "OUTCAR.h2o.md", fmt="vasp/outcar" | ||
| ) | ||
| absolute = dpdata.MultiSystems.from_dir( | ||
| os.path.abspath("poscars"), "OUTCAR.h2o.md", fmt="vasp/outcar" | ||
| ) | ||
| self.assertEqual(len(relative), 1) | ||
| self.assertEqual(len(absolute), len(relative)) | ||
| self.assertEqual(absolute.get_nframes(), relative.get_nframes()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert equivalence of the matched systems, not only aggregate counts.
These assertions would pass if the absolute-path search returned a different set of systems with the same length and total frame count. Compare system names and each system’s frame count (or use the repository’s existing system-comparison helper) to fully cover the stated relative/absolute equivalence requirement.
🤖 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_multisystems.py` around lines 54 - 64, Strengthen
test_absolute_directory_matches_relative_directory in TestMultiSystemsFromDir to
compare the matched systems themselves, not just collection and aggregate frame
counts. Assert equivalent system names and per-system frame counts, or reuse the
repository’s existing system-comparison helper, while preserving the
relative-versus-absolute directory setup.
Merging this PR will improve performance by 13.33%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_import |
12.6 ms | 11.1 ms | +13.33% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing njzjz-bot:fix/issue-986 (e9bd912) with master (713ef68)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1015 +/- ##
==========================================
+ Coverage 86.95% 87.01% +0.06%
==========================================
Files 90 90
Lines 8330 8330
==========================================
+ Hits 7243 7248 +5
+ Misses 1087 1082 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Approving: both fixes are correct and CI is green.
- The first-append
deepcopy(system.data)completes the "do not modify the appended system" invariant from #483, which deep-copied in every branch except this one (the shallowdict.copy()left arrays/lists aliased). - The
from_dirchange toos.path.join(dir_name, "**", file_name)matches the form originally suggested in #63 and fixes absolute directory paths (the./prefix turned them into no-match relative patterns) without regressing relative paths.
Note: the append deepcopy here is exactly the fix that #1014's currently-failing test_first_append_does_not_alias_source needs. These two PRs are complementary — this one has the fix, #1014 has the test — so they should be sequenced/merged together.
2 participants
Fixes #986.
Deep-copy the first appended system and build absolute-safe recursive glob patterns in MultiSystems.from_dir.
Tests:
cd tests && python -m unittest test_multisystems.TestMultiSystemsFromDir test_system_appendWhy existing tests missed it: There was no first-append ownership test and no absolute-directory from_dir case.
Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
Bug Fixes
Tests