Skip to content

Commit

Permalink
Tests: Fix StructureData test breaking for recent pymatgen versio…
Browse files Browse the repository at this point in the history
…ns (#6088)

The roundtrip test for the `StructureData` class using `pymatgen`
structures as a go between started failing. The structure is constructed
from a CIF file with partial occupancies. The `label` attribute of each
site in the pymatgen structure, as returned by `as_dict` would look like
the following, originally:

    ['Bi', 'Bi', 'Te:0.667, Se:0.333', 'Te:0.667, Se:0.333', 'Te:0.667, Se:0.333']
    ['Bi', 'Bi', 'Te:0.667, Se:0.333', 'Te:0.667, Se:0.333', 'Te:0.667, Se:0.333']

In commit 63bbd23b57ca2c68eaca07e4915a70ef66e13405, released with
v2023.7.14, the CIF parsing logic in `pymatgen` was updated to include
parsing of the atom site labels and store them on the site `label`
attribute. This would result in the following site labels for the
structure parsed directly from the CIF and the one after roundtrip
through `StructureData`:

    ['Bi', 'Bi', 'Se1', 'Se1', 'Se1']
    [None, None, None, None, None]

The roundtrip returned `None` values because in the previously mentioned
commit, the newly added `label` property would return `None` instead of
the species label that used to be returned before. This behavior was
corrected in commit 9a98f4ce722299d545f2af01a9eaf1c37ff7bd53 and released
with v2023.7.20, after which the new behavior is the following:

    ['Bi', 'Bi', 'Se1', 'Se1', 'Se1']
    ['Bi', 'Bi', 'Te:0.667, Se:0.333', 'Te:0.667, Se:0.333', 'Te:0.667, Se:0.333']

The site labels parsed from the CIF are not maintained in the roundtrip
because the `StructureData` does not store them. Therefore when the final
pymatgen structure is created from it, the `label` is `None` and so
defaults to the species name.

Since the label information is not persisted in the `StructureData` it
is not guaranteed to be maintained in the roundtrip and so it is
excluded from the test.

Cherry-pick: d1d64e8
  • Loading branch information
sphuber committed Nov 15, 2023
1 parent 5cf3d1d commit 093037d
Showing 1 changed file with 11 additions and 1 deletion.
12 changes: 11 additions & 1 deletion tests/test_dataclasses.py
Original file line number Diff line number Diff line change
Expand Up @@ -2173,6 +2173,16 @@ def test_1(self):
dict1 = pymatgen_struct.as_dict()
dict2 = pymatgen_struct_roundtrip.as_dict()

# In pymatgen v2023.7.14 the CIF parsing was updated to include the parsing to atomic site labels. However, this
# information is not stored in the ``StructureData`` and so the structure after the roundtrip uses the default
# which is the specie name. The latter is correct in that it reflects the partial occupancies, but it differs
# from the labels parsed from the CIF which is simply parsed as ``Se1`` causing the test to fail. Since the
# site label information is not stored in the ``StructureData`` it is not possible to preserve it in the
# roundtrip and so it is excluded from the check.
for dictionary in [dict1, dict2]:
for site in dictionary['sites']:
site.pop('label', None)

for i in dict1['sites']:
i['abc'] = [round(j, 2) for j in i['abc']]
for i in dict2['sites']:
Expand All @@ -2192,7 +2202,7 @@ def recursively_compare_values(left, right):
elif isinstance(left, float):
testing.assert_almost_equal(left, right)
else:
assert left == right, f'{value} is not {right}'
assert left == right, f'{left} is not {right}'

recursively_compare_values(dict1, dict2)

Expand Down

0 comments on commit 093037d

Please sign in to comment.