Skip to content

Reimplement from_ase_atoms_list with parallel Alchemi-based graph construction #154

Merged
vsimkus merged 5 commits intoorbital-materials:mainfrom
zdcao121:feat/parallel-from-ase-atoms-list
Mar 15, 2026
Merged

Reimplement from_ase_atoms_list with parallel Alchemi-based graph construction #154
vsimkus merged 5 commits intoorbital-materials:mainfrom
zdcao121:feat/parallel-from-ase-atoms-list

Conversation

@zdcao121
Copy link
Copy Markdown
Contributor

@zdcao121 zdcao121 commented Mar 14, 2026

Summary

  • Override from_ase_atoms_list in ForcefieldAtomsAdapter to leverage batch_compute_pbc_radius_graph for parallel Alchemi-based
    graph construction
  • Extracts positions, cells, pbcs, and atomic numbers from all ASE atoms, concatenates into batched tensors, and processes them in
    parallel instead of one by one
  • Preserves ASE-specific features: fix_atoms constraints, tags, charge/spin

Closes #152

Test plan

  • All existing adapter tests pass
  • Equivalence with sequential approach (matching n_node, n_edge, positions, atomic_numbers, edge distances)
  • Non-periodic systems
  • Single-atom list falls back to from_ase_atoms
  • Empty list raises ValueError
  • Charge/spin handling across batched atoms

🤖 Generated with Claude Code

…struction

Override from_ase_atoms_list in ForcefieldAtomsAdapter to leverage
batch_compute_pbc_radius_graph for parallel graph construction, instead
of processing atoms one by one sequentially.

Closes orbital-materials#152

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Test equivalence with sequential processing, non-periodic systems,
single-atom fallback, empty list error, and charge/spin handling.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@vsimkus vsimkus left a comment

Choose a reason for hiding this comment

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

Looking good, thank you!

Can you change the charge/spin behavior to all-or-nothing, as detailed in my comment, raising an error if some atoms include charge/spin but others don't.

Also, a few test improvemens and linting errors should be fixed.

zdcao121 and others added 2 commits March 15, 2026 12:48
- Change charge/spin to all-or-nothing: raise ValueError if only some
  atoms have charge/spin instead of using default values
- Move AtomGraphs import to top of test file
- Add sequential equivalence comparison in non-periodic test
- Add senders/receivers/vectors assertions in single-atom fallback test
- Add test for mixed charge/spin raising error

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add strict=True to zip() call
- Apply ruff format to both source and test files

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@zdcao121 zdcao121 requested a review from vsimkus March 15, 2026 08:06
Copy link
Copy Markdown
Contributor

@vsimkus vsimkus left a comment

Choose a reason for hiding this comment

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

Looks great @zdcao121 👍 Thanks for the contribution!

@vsimkus vsimkus merged commit b0ace47 into orbital-materials:main Mar 15, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

reimplementation of from_ase_atoms_list method in the ForcefieldAtomsAdapter class

2 participants