Skip to content

Commit dd867f7

Browse files
authored
fix(corrections): swap defect/bulk site potentials in get_efnv_correction (GH#219) (#226)
Lines 96-97 of kumagai.py had defect_potentials and bulk_potentials sourced from the wrong structures. For antisite defects (defect.num_sites == bulk.num_sites) the swap was silent but produced incorrect alignment; for vacancies and interstitials it raised IndexError downstream in pydefect's make_efnv_correction. - Restore correct ordering: potentials are read from the structure they describe. - The pre-existing test_kumagai assertion (q=+1 correction > 0) was effectively locking in the buggy behavior — the correct value with proper ordering is approximately -0.49 eV. Updated to assert the new reference value. - Add test_kumagai_vacancy regression test that synthesizes a vacancy (different num_sites between defect and bulk) and exercises the bug directly. Without the fix this raises the same IndexError reported in GH#219. Reported by user in GH issue #219; the issue body identifies the swap and the verified-against-legacy KumagaiCorrection value.
1 parent 12c1f80 commit dd867f7

2 files changed

Lines changed: 47 additions & 3 deletions

File tree

pymatgen/analysis/defects/corrections/kumagai.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,8 @@ def get_efnv_correction(
9393
"""
9494
_check_import_pydefect()
9595
# ensure that the structures have the "potential" site property
96-
bulk_potentials = [site.properties["potential"] for site in defect_structure]
97-
defect_potentials = [site.properties["potential"] for site in bulk_structure]
96+
defect_potentials = [site.properties["potential"] for site in defect_structure]
97+
bulk_potentials = [site.properties["potential"] for site in bulk_structure]
9898

9999
defect_calc_results = CalcResults(
100100
structure=defect_structure,

tests/test_corrections.py

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import math
2+
13
import pytest
24
from pymatgen.analysis.defects.corrections.freysoldt import (
35
get_freysoldt_correction,
@@ -85,10 +87,52 @@ def test_kumagai(test_dir):
8587
)
8688
assert res0.correction_energy == pytest.approx(0, abs=1e-4)
8789

90+
# Bug-fix invariant (GH#219): pre-fix code returned a positive value
91+
# because defect/bulk site potentials were swapped at extraction. The
92+
# corrected wrapper must yield a finite, negative value for this q=+1
93+
# antisite fixture.
8894
res1 = get_efnv_correction(
8995
1, sd1, sb, dielectric_tensor=[[1, 0, 0], [0, 1, 0], [0, 0, 1]]
9096
)
91-
assert res1.correction_energy > 0
97+
assert math.isfinite(res1.correction_energy)
98+
assert res1.correction_energy < 0
99+
100+
# Snapshot value generated 2026-06-12 against the bundled Mg_Ga fixture.
101+
# Locks in pydefect numerics for regression detection only; not compared
102+
# against an external reference. Note the identity dielectric tensor —
103+
# chosen for fixture stability, not physical realism. Update if pydefect
104+
# numerics drift.
105+
assert res1.correction_energy == pytest.approx(-0.4898778, abs=1e-4)
106+
107+
108+
def test_kumagai_vacancy(test_dir):
109+
"""Regression test for GH#219: site potentials must not be swapped.
110+
111+
With antisite defects (defect.num_sites == bulk.num_sites) the swap of
112+
`defect_structure` and `bulk_structure` on the `site.properties["potential"]`
113+
extraction is silent. With vacancies / interstitials the lengths differ and
114+
the bug surfaces as an IndexError downstream in pydefect.
115+
116+
We synthesize a vacancy by removing a single site from the antisite q=0
117+
Mg_Ga test structure, so the defect has 31 sites and bulk has 32. The
118+
correction itself is meaningless for this synthetic input, but the call
119+
must not raise IndexError, and the potentials must be sourced from the
120+
correct structure.
121+
"""
122+
sb = get_structure_with_pot(test_dir / "Mg_Ga" / "bulk_sc")
123+
sd0 = get_structure_with_pot(test_dir / "Mg_Ga" / "q=0")
124+
# Synthesize a vacancy: drop one site from the defect structure.
125+
sd_vac = sd0.copy()
126+
sd_vac.remove_sites([0])
127+
assert len(sd_vac) != len(sb), "test setup invalid: lengths must differ"
128+
129+
# With the swap bug, building `defect_potentials` from `bulk_structure`
130+
# produces a 32-element array while `defect_structure` only has 31 sites,
131+
# which raises IndexError inside pydefect's make_efnv_correction. The
132+
# bug is fixed if this call returns without raising.
133+
get_efnv_correction(
134+
0, sd_vac, sb, dielectric_tensor=[[1, 0, 0], [0, 1, 0], [0, 0, 1]]
135+
)
92136

93137

94138
def test_kumagai_missing():

0 commit comments

Comments
 (0)