Skip to content

fix: preserve atom order in map_atom_types#1007

Open
rathodkunj2005 wants to merge 1 commit into
deepmodeling:masterfrom
rathodkunj2005:fix-map-atom-types-order
Open

fix: preserve atom order in map_atom_types#1007
rathodkunj2005 wants to merge 1 commit into
deepmodeling:masterfrom
rathodkunj2005:fix-map-atom-types-order

Conversation

@rathodkunj2005

@rathodkunj2005 rathodkunj2005 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • map each existing atom type entry through atom_names instead of expanding grouped atom_numbs
  • add a regression test for interleaved atom order ([H, O, H])

Fixes #984.

Verification

  • python -m pytest test_system_set_type.py -q (from tests/): 3 passed in 0.20s
  • python -m ruff check dpdata/system.py tests/test_system_set_type.py: All checks passed!
  • issue reproducer now prints [0, 1, 0]

Summary by CodeRabbit

  • Bug Fixes

    • Improved atom-type remapping so it now preserves the current atom order when converting to a new type map.
    • Ensured remapped atom types are generated consistently from existing atom-type indices.
  • Tests

    • Added coverage verifying atom-type remapping returns the expected order for a simple in-memory system.

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. dpdata labels Jul 3, 2026
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5ac78019-0bea-4bc0-80b5-b0d3960d54b8

📥 Commits

Reviewing files that changed from the base of the PR and between 5de0229 and 139e559.

📒 Files selected for processing (2)
  • dpdata/system.py
  • tests/test_system_set_type.py

📝 Walkthrough

Walkthrough

Fixed System.map_atom_types in dpdata/system.py to compute mapped atom types directly from the existing atom_types index sequence via atom_names, instead of rebuilding from grouped atom_names/atom_numbs, preserving current atom order. Added a corresponding unit test.

Changes

map_atom_types Fix

Layer / File(s) Summary
Order-preserving atom type mapping
dpdata/system.py, tests/test_system_set_type.py
map_atom_types now indexes atom_names using existing atom_types entries and maps through type_map, preserving atom order; a new test validates the corrected mapping.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Estimated code review effort

Not applicable beyond the metadata line above.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main fix: preserving atom order in map_atom_types.
Linked Issues check ✅ Passed The code now maps self.data["atom_types"] directly through atom_names, matching #984 and the expected interleaved order.
Out of Scope Changes check ✅ Passed The changes stay focused on the atom-type remapping fix and its regression test.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codspeed-hq

codspeed-hq Bot commented Jul 3, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 2 untouched benchmarks


Comparing rathodkunj2005:fix-map-atom-types-order (139e559) with master (5de0229)

Open in CodSpeed

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.87%. Comparing base (5de0229) to head (139e559).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1007      +/-   ##
==========================================
- Coverage   86.87%   86.87%   -0.01%     
==========================================
  Files          89       89              
  Lines        8268     8266       -2     
==========================================
- Hits         7183     7181       -2     
  Misses       1085     1085              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dpdata size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code scan] System.map_atom_types ignores the current atom order

1 participant