Skip to content

fix(system): copy first append and support absolute paths #1015

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

Merged
njzjz merged 2 commits into from
Jul 18, 2026
Merged

fix(system): copy first append and support absolute paths #1015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebab942
fix(system): copy first append and support absolute paths
Jul 16, 2026
e9bd912
Merge branch 'master' into fix/issue-986
wanghan-iapcm Jul 16, 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
9 changes: 7 additions & 2 deletions dpdata/system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,10 @@ def append(self, system: System) -> bool:
return False
elif not len(self.data["atom_numbs"]):
# this system is non-converged but the system to append is converged
self.data = system.data.copy()
# A shallow dict copy would still alias all arrays and lists in
# ``system.data``. The first append must have the same ownership
# semantics as subsequent append operations.
self.data = deepcopy(system.data)
return False
if system.uniq_formula != self.uniq_formula:
raise RuntimeError(
Expand Down Expand Up @@ -1492,8 +1495,10 @@ def from_dir(
type_map: list[str] | None = None,
):
multi_systems = cls()
# Do not prepend ``./``: doing so turns an absolute directory into a
# relative pattern and silently yields no matches.
target_file_list = sorted(
glob.glob(f"./{dir_name}/**/{file_name}", recursive=True)
glob.glob(os.path.join(dir_name, "**", file_name), recursive=True)
)
for target_file in target_file_list:
multi_systems.append(
Expand Down
13 changes: 13 additions & 0 deletions tests/test_multisystems.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,19 @@ def test_str(self):
)


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())
Comment on lines +54 to +64

@coderabbitai coderabbitai Bot Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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.



class TestMultiSystemsAdd(unittest.TestCase, CompLabeledSys, MultiSystems, IsNoPBC):
def setUp(self):
self.places = 6
Expand Down
Loading