Skip to content

fix(system): isolate sliced system metadata #1014

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.

Sign up for GitHub

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

Open
njzjz-bot wants to merge 3 commits into
base: master
Choose a base branch
from
+44 −3
Open

fix(system): isolate sliced system metadata #1014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8a6b8b9
fix(system): isolate sliced system metadata
Jul 16, 2026
527e2d4
fix(system): deep-copy first appended system
Jul 21, 2026
8969202
chore: merge master into fix/issue-985
Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions dpdata/system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,10 +461,16 @@ def sub_system(self, f_idx: int | slice | list | np.ndarray):
slice(None) for _ in self.data[tt.name].shape
]
new_shape[axis_nframes] = f_idx
tmp.data[tt.name] = self.data[tt.name][tuple(new_shape)]
# A slice produces a NumPy view, while advanced indexing
# produces a copy. Deep-copy the result so ownership is
# consistent for every supported frame selector.
tmp.data[tt.name] = deepcopy(self.data[tt.name][tuple(new_shape)])

@wanghan-iapcm wanghan-iapcm Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix verified -- test_sub_system_does_not_alias_metadata fails at the merge base (AssertionError: ['X'] != ['H']) and passes here, and I confirmed atom_names, atom_types, coords and cells all leaked pre-fix. Thanks for closing this properly.

One tradeoff worth reconsidering. The comment above is right that advanced indexing already produces a copy -- which means for those selectors this deepcopy is a second full memcpy of the frame data. Measured on a 20k-frame x 100-atom system, merge base vs this head:

operation base head ratio
sub_system(permutation) 0.021 s 0.040 s 1.9x
sub_system(list) 0.0021 s 0.0099 s 4.7x
sub_system(slice) ~0.00003 s 0.018 s ~600x
shuffle() 0.021 s 0.039 s 1.87x

System.shuffle(), to("list") (which slices per frame), and mixed-format train/test splitting all sit on this path. The CodSpeed suite only covers test_import / test_cli, so it will not flag this.

Copying only when the result actually aliases the source keeps the fix and drops the redundancy:

res = self.data[tt.name][tuple(new_shape)]
if np.shares_memory(res, self.data[tt.name]):
    res = res.copy()
tmp.data[tt.name] = res

Note that swapping deepcopy for .copy() on its own buys nothing -- I measured 632 us vs 638 us on a (1000,50,3) array. The cost is the redundant copy, not deepcopy. The frame-independent branch below should keep its unconditional deepcopy; that one is the actual #985 fix.

Minor, for the record: test_first_append_does_not_alias_source passes at the merge base, because the first-append deepcopy landed on master via #1015 (2cd82ee) before this branch's merge base -- so it arrived here through the master merge rather than from 527e2d4. Harmless redundant coverage, but test_sub_system_does_not_alias_metadata is the test actually guarding this change.

else:
# keep the original data
tmp.data[tt.name] = self.data[tt.name]
# Frame-independent values (atom names/types, ``orig``, and
# optional metadata) are usually lists or arrays. Copy them
# as well as frame data so editing a slice can never mutate
# the source system through a shared mutable object.
tmp.data[tt.name] = deepcopy(self.data[tt.name])
return tmp

def append(self, system: System) -> bool:
Expand Down
35 changes: 35 additions & 0 deletions tests/test_system_append.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,41 @@ def test_failed_append(self):
)


class TestAppendOwnership(unittest.TestCase):
"""Regression tests for copy-on-slice and copy-on-first-append semantics."""

def test_sub_system_does_not_alias_metadata(self):
system = dpdata.System(
data={
"atom_names": ["H"],
"atom_numbs": [1],
"atom_types": np.array([0]),
"orig": np.zeros(3),
"cells": np.eye(3).reshape(1, 3, 3),
"coords": np.zeros((1, 1, 3)),
}
)
sub = system[0:1]
sub.data["atom_names"][0] = "X"
sub.data["atom_types"][0] = 1
sub.data["coords"][0, 0, 0] = 123.0
sub.data["cells"][0, 0, 0] = 456.0
self.assertEqual(system.data["atom_names"], ["H"])
np.testing.assert_array_equal(system.data["atom_types"], [0])
self.assertEqual(system.data["coords"][0, 0, 0], 0.0)
self.assertEqual(system.data["cells"][0, 0, 0], 1.0)

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")
Comment thread
njzjz-bot marked this conversation as resolved.
self.assertNotEqual(target.data["coords"][0, 0, 0], 123.0)

Comment thread
coderabbitai[bot] marked this conversation as resolved.

class TestVaspXmlAppend(unittest.TestCase, CompLabeledSys, IsPBC):
def setUp(self):
self.places = 6
Expand Down
Loading