feat: Add functions for getting information from `Structure` by cadenmyers13 · Pull Request #187 · diffpy/diffpy.structure

cadenmyers13 · 2026-03-24T14:48:19Z

See news file for all the methods that are added.

…ve tests

codecov · 2026-03-24T15:28:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.21%. Comparing base (c5659e9) to head (65d2729).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
+ Coverage   99.20%   99.21%   +0.01%     
==========================================
  Files          15       15              
  Lines        2507     2554      +47     
==========================================
+ Hits         2487     2534      +47     
  Misses         20       20

Files with missing lines	Coverage Δ
tests/test_structure.py	`99.82% <100.00%> (+0.01%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tests/test_structure.py

cadenmyers13 · 2026-03-24T17:19:49Z

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

sbillinge · 2026-03-25T12:11:13Z

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 · 2026-03-25T14:48:20Z

@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:

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
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?

sbillinge · 2026-03-26T07:30:47Z

The main question is serialization. I think that if it is possible to serialize ASE objects we can read them without an ASE dependency. We can also have code to write them.

Your UCs are good for some kind of high throughout pipeline where ASE is generating large numbers of objects that are being passed to Diffpy and serialization would be a serious performance bottleneck. However I do see the use and convenience. It occured to me that we could make an ASE pack so we don't pick up a new dependency in the core.

sbillinge · 2026-03-26T07:34:03Z

I think that thinking of actual UCs could be good. The one highest on my list would be to take an MD simulation and Compute the PDF. Another would be to support something like Soham's cluster mining. We should maybe check how he did it as he may already have some code. I would guess that the code is on gitlab

sbillinge · 2026-03-26T07:35:19Z

For these UCs do you know if it is Atoms objects or Some other kind of ASE objects that is passed?

…apter

cadenmyers13 · 2026-04-02T16:20:19Z

@sbillinge I moved the discussion from here to the associated issue. Ive changed this PR to only include the function for getting structural information

sbillinge

this looks good. Please see comments.

sbillinge · 2026-04-03T11:24:28Z

tests/test_structure.py

 # End of class TestStructure


+def test_get_chemical_symbols(datafile):


do we need the case where the symbols are in there as ions? In that case I guess the desired behavior is that this returns the list without cleaning the ionic charge, right?

Then there may be a different function that cleans them.

In the future, we may want to handle ions differently than non ions, so (and i am not sure what the actual behavior is because I don't see a test for it) it would be better if this getter returned them uncleaned and the cleaning step was separate for when we want to clean (i.e., atm). Does it make sense?

@sbillinge Yes makes sense. all atoms get passed through the atom_bare_symbol function which strips all the symbols and only returns species. The challenge with supporting ions is sometimes the atom passed through is a bare symbol, ion, or something else. For example, the 12-C input for this test,

@pytest.mark.parametrize( "symbol, expected", [ ("Cl-", "Cl"), ("Ca2+", "Ca"), ("12-C", "C"), ], ) def test_atom_bare_symbol(symbol, expected): actual = atom_bare_symbol(symbol) assert actual == expected

I think its better to put this on a separate PR/issue. I'll make that issue

sbillinge · 2026-04-03T11:26:02Z

tests/test_structure.py

+
+@pytest.mark.parametrize(
+    "return_array",
+    [  # case: user wants ADPs as an array


nice, I like these cases

sbillinge · 2026-04-03T11:30:09Z

tests/test_structure.py

+    else:
+        expected_displacement = {
+            # Iodine
+            "I_8_11": np.float64(0.025),


Your test seems to be requiring np.float64 types. Is this behavior you want? It seems oddly specific. Remember, tests are supposed to test desired ``behavior". Is it desired behavior that we want it to return float64?

Yeah probably not desired to have np.float64. I just incorporated this because this was in the output. Ive removed it.

sbillinge · 2026-04-03T11:31:59Z

tests/test_structure.py

+    "return_array",
+    [  # case: user wants isotropic displacement parameters as an array
+        # expected: a 1D array of shape (num_atoms,) with the Uiso values
+        True,


this is a slightly odd and non standard use of parametrize. We normally give ([inputs], [outputs]) which is more extensible and more readable by a human, and then the test itself is very short and has little or no logic in it.

Okay I updated it. I still included the logic because the assert statements vary slightly as == isnt supported for numpy arrays

sbillinge · 2026-04-03T11:32:10Z

tests/test_structure.py

+        assert np.allclose(actual_isotropic_displacement, expected_isotropic_displacement)
+    else:
+        expected_isotropic_displacement = {
+            "Pb_1_Uiso": np.float64(0.0225566),


see comment above.

cadenmyers13 · 2026-04-03T18:31:44Z

@sbillinge ready for review. responded to comments.

cadenmyers13 added 10 commits March 23, 2026 16:24

get_chemical_symbols function and test

b252ea3

get_fractional_coordinates code and test

f339f4e

get_cartesian_coordinates function and test

37f6426

get_isotropic_displacement_parameters tests and func

48c115e

get_occupancies test and func

1f51263

add get_lattice_vectors, get_lattice_vector_angles and their respecti…

bd4a6c0

…ve tests

news

67d978a

make ase converter take str or list of strs

bb207e0

update cartesian coordinate getter so it doesnt round

7199c46

add ase to conda.txt and pip.txt

8fa8327

add option to return ADPs as an array or dictionary

beec69c

cadenmyers13 commented Mar 24, 2026

View reviewed changes

tests/test_structure.py Outdated Show resolved Hide resolved

fix minor bug in typeerror test

cfae583

cadenmyers13 added 6 commits April 2, 2026 11:57

Merge branch 'main' of github.com:diffpy/diffpy.structure into ase-ad…

8354820

…apter

rm ase adapter tests

f7f5466

rm ase adapter function

c24fd70

update news

30872d9

rm ase from requirements

a018a30

remove ase fixtures

0e5cc9d

cadenmyers13 changed the title ~~feat: Add structure adapter to convert ASE.Atom objects to diffpy.structure.Structure objects and additional useful methods~~ feat: Add functions for getting information from Structure Apr 2, 2026

cadenmyers13 mentioned this pull request Apr 2, 2026

feat: structure object to ASE structure objects adapter #127

Open

sbillinge reviewed Apr 3, 2026

View reviewed changes

cadenmyers13 added 3 commits April 3, 2026 14:09

rm np.float64 requirement from test

6259c38

add input,expected to tests for adp getters

47b440d

precommit edits

65d2729

		# End of class TestStructure


		def test_get_chemical_symbols(datafile):

Conversation

cadenmyers13 commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

cadenmyers13 commented Mar 24, 2026

Uh oh!

sbillinge commented Mar 25, 2026

Uh oh!

cadenmyers13 commented Mar 25, 2026

Uh oh!

sbillinge commented Mar 26, 2026

Uh oh!

sbillinge commented Mar 26, 2026

Uh oh!

sbillinge commented Mar 26, 2026

Uh oh!

cadenmyers13 commented Apr 2, 2026

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cadenmyers13 commented Mar 24, 2026 •

edited

Loading

codecov bot commented Mar 24, 2026 •

edited

Loading