fix(system): isolate sliced system metadata#1014
Conversation
Deep-copy both frame-selected arrays and frame-independent metadata when creating a subsystem so slice views and mutable values cannot alias the source. Add regressions for coordinates, cells, names, and atom types, which existing comparison tests did not mutate. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
dosubot
Bot
added
size:S
Merging this PR will not alter performance
|
📝 WalkthroughWalkthrough
ChangesSystem ownership isolation
Estimated code review effort: 3 (Moderate) | ~20 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_system_append.py`:
- Around line 45-54: Update System.append() so its first-append path deep-copies
system.data instead of using a shallow dictionary copy, ensuring nested lists
and NumPy arrays do not alias the source. Preserve the existing append behavior
for non-empty systems.
🪄 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: c713429b-f5d4-4b18-b072-81d23c410825
📒 Files selected for processing (2)
dpdata/system.pytests/test_system_append.py
| def test_first_append_does_not_alias_source(self): | ||
| source = dpdata.System("poscars/POSCAR.oh.d", fmt="vasp/poscar") | ||
| target = dpdata.System() | ||
| target.append(source) | ||
|
|
||
| source.data["atom_names"][0] = "X" | ||
| source.data["coords"][0, 0, 0] = 123.0 | ||
| self.assertEqual(target.data["atom_names"][0], "O") | ||
| self.assertNotEqual(target.data["coords"][0, 0, 0], 123.0) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing implementation for first-append deep-copy.
While this test correctly asserts that appending to an empty system should deep-copy the source data to prevent aliasing, the corresponding fix in dpdata/system.py was omitted from this PR. Currently, System.append() uses self.data = system.data.copy(), which performs a shallow copy of the dictionary. Consequently, lists and numpy arrays remain aliased, and this test will fail when executed.
To fix this, update System.append() in dpdata/system.py (around line 489) to use deepcopy:
elif not len(self.data["atom_numbs"]):
# this system is non-converged but the system to append is converged
- self.data = system.data.copy()
+ self.data = deepcopy(system.data)
return FalseBased on the PR objectives, the first append into an empty system must deep-copy source data to prevent mutations from propagating.
🤖 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_system_append.py` around lines 45 - 54, Update System.append() so
its first-append path deep-copies system.data instead of using a shallow
dictionary copy, ensuring nested lists and NumPy arrays do not alias the source.
Preserve the existing append behavior for non-empty systems.
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Requesting changes: this fixes the sub_system half of #985 but leaves the first-append()-into-empty path aliased, so the PR's own test_first_append_does_not_alias_source fails and CI (build 3.10 / 3.13) is red. See the inline note for the one-line fix.
|
|
||
| source.data["atom_names"][0] = "X" | ||
| source.data["coords"][0, 0, 0] = 123.0 | ||
| self.assertEqual(target.data["atom_names"][0], "O") |
There was a problem hiding this comment.
test_first_append_does_not_alias_source exercises the first-append()-into-empty path, but that path was not changed — dpdata/system.py:489 still does self.data = system.data.copy(), a shallow dict.copy() that keeps the source's arrays/lists aliased. So this test fails as shipped (CI build 3.10 / 3.13 is red). Applying self.data = deepcopy(system.data) there completes the #985 fix and turns the test green.
2 participants
Fixes #985.
Deep-copy frame-selected arrays and frame-independent metadata so subsystem slices cannot mutate their source through NumPy views or shared lists.
Tests:
cd tests && python -m unittest test_system_appendWhy existing tests missed it: The existing tests only compared sliced values; they never mutated a slice, so aliasing remained invisible.
Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
Bug Fixes
Tests