Fix bug in feff inputs.py

[kaifengZheng]

Summary

I found a small bug inside the feff.inputs module. The absorbing_atom index is misused in the following sentence, which generated a bug.

atom_sym = get_absorbing_atom_symbol_index(absorbing_atom, self._cluster)[0]

When I created a cluster using inputs.Atoms,there is an error, since absorbing_atom in the argument is the index of the absorbing atom in the original structure, but the above sentence misused this index for the absorbing atom in the generated cluster, which should be 0. If we create a particle that has fewer atoms than the assigned index of the absorbing atom in the original structure, it can generate an IndexError.

For example:
the original structure has 55 U atoms and assigns a U atom with index 54 as the absorber, but we generate a particle that only has 30 atoms, so the index is mismatched and will prompt the following error.

I resubmitted this PR, since the previous one shows too many conflicts.
Major changes:

• feature: fix encountered IndexError error
• fix 1: delete

  atom_sym = get_absorbing_atom_symbol_index(absorbing_atom, self._cluster)[0]

Firstly, I changed this absorbing_atom argument to 0. However, after scrutinizing the codes, I found that this sentence was redundant, so I deleted it.

• fix 2: modify one line:

     self.pot_dict = get_atom_map(self._cluster, self.absorbing_atom)

• add test file and updae test_set.py with a test function

Todos

If this is work in progress, what else needs to be done?

• feature 2: ...
• fix 2:

Checklist

• Google format doc strings added. Check with ruff.
• Type annotations included. Check with mypy.
• Tests added for new features/fixes.
• If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.57% ⚠️

Comparison is base (5f369ec) 74.69% compared to head (0018837) 74.12%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3256      +/-   ##
==========================================
- Coverage   74.69%   74.12%   -0.57%     
==========================================
  Files         230      230              
  Lines       69375    69374       -1     
  Branches    16154    16154              
==========================================
- Hits        51818    51423     -395     
- Misses      14490    14918     +428     
+ Partials     3067     3033      -34     

Files Changed	Coverage Δ
pymatgen/io/feff/inputs.py	82.69% <100.00%> (-0.04%)	⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[janosh]

Thanks for the fix @kaifengZheng! Do we need a new CIF file? Maybe some of the existing ones could be used instead?

[janosh]

I don't see how this test is related to your change in pymatgen/io/feff/inputs.py. The past passes with or without your change.

[kaifengZheng]

On my side, using the original code:

[janosh]

Try running the actual test with the old code

pytest tests/io/feff/test_sets.py

[kaifengZheng]

This error happens when the absorbing atom index exceeds the number of atoms in the created cluster. Thus for any structure with many absorbers(same type of atom). We want to use the program to create a small particle surrounding the absorber, which has a big index. The program will fail.

[kaifengZheng]

[kaifengZheng]

After change the code:

[kaifengZheng]

By the way, to show the error, we need the Oxygen block behind the Fe:

If the order is reversed, I think we need to change the absorber atom to Fe to fulfill this test:

def test_cluster_index(self):
        cif_file = f"{TEST_FILES_DIR}/Fe3O4.cif"
        structure = CifParser(cif_file).get_structures()[0]
        for i in range(len(structure.species)):
            if structure.species[i] == Element("Fe"):
                assert Atoms(structure, i, 3).cluster

[kaifengZheng]

Please try this time. I removed choosing Element part.

[janosh]

Thanks for the new test. Just confirmed that it covers the fix. 👍

[kaifengZheng]

Thanks for the merge @janosh!