Skip to content

feat: Add structure adapter to convert ASE.Atom objects to diffpy.structure.Structure objects and additional useful methods#187

Open
cadenmyers13 wants to merge 12 commits intodiffpy:mainfrom
cadenmyers13:ase-adapter
Open

feat: Add structure adapter to convert ASE.Atom objects to diffpy.structure.Structure objects and additional useful methods#187
cadenmyers13 wants to merge 12 commits intodiffpy:mainfrom
cadenmyers13:ase-adapter

Conversation

@cadenmyers13
Copy link
Contributor

@cadenmyers13 cadenmyers13 commented Mar 24, 2026

See news file for all the methods that are added.

This converts ase --> diffpy.structure. The method also includes the optional lost_info input. Here, the user can provide an attribute or method of the ase.Atoms object as a string or list of strings to get additional information that is available in ase that is not supported in diffpy.structure (magnetic moments, velocities, etc).

@codecov
Copy link

codecov bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.99%. Comparing base (39d41f0) to head (cfae583).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
+ Coverage   98.93%   98.99%   +0.05%     
==========================================
  Files          13       13              
  Lines        1884     1987     +103     
==========================================
+ Hits         1864     1967     +103     
  Misses         20       20              
Files with missing lines Coverage Δ
tests/conftest.py 76.31% <100.00%> (+23.68%) ⬆️
tests/test_structure.py 99.81% <100.00%> (+0.03%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

# We set the expected error message to None because this expectation is
# out of our control, but it is good to make sure that we are
# raising the error from ASE.
(["get_magnetic_moments"], RuntimeError, None),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This basically is a test for ase and not diffpy.structure. do you think its okay to leave this?

@cadenmyers13
Copy link
Contributor Author

@sbillinge ready for review. ase to diffpy.structure adapter.

@sbillinge
Copy link
Contributor

Let's chat about this. It seems that adding ase as a dependency is a big step and we need to understand better the use-cases. Let's start with use-cases, then we can work on design and tests. Then we can code. Especially when we have AI to write the code, this process is the most important because the code-writing is the lowest cost now and the biggest risk is to introduce bloat and entropy because the AI doesn't care too much about that it seems like.

@cadenmyers13
Copy link
Contributor Author

@sbillinge Here's the UCs i thought of and is what guided the code on this PR. Only option 2 is included here. Option 1 would be on a different PR. Both 1a and 1b would be things that would need to be implemented in ase, but i included them here for completion:

  1. User wants to convert diffpy --> ase
    a. User calls a method in diffpy.structure.Structure and passes diffpy obj to convert diffpy --> ASE, or
    b. User calls a method in ASE.Atoms and passes diffpy obj to convert a diffpy --> ASE

  2. User want to convert ase --> diffpy
    a. User calls a method in diffpy.structure.Structure and passes ase obj to convert diffpy --> ASE, or
    b. User calls a method in ASE.Atoms and passes ase obj to convert a diffpy --> ASE

Any other use cases you could think of?

Also, we might be able to get away with this without adding it as a dependency. The only use for it is an isinstance check that raises an error, but we could probably remove this and the user would get an error downstream generated by something else. Presumably, if you're trying to convert a diffpy object to ase you already have ase installed in your env. Idk how removing this will go over in testing though. We could just add ase to tests.txt, right?

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.

2 participants