pymatgen
pymatgen copied to clipboard
materialsproject
Bug in `ResWriter._ions_from_sites` in `pymatgen.io.res`
Python version
Python 3.10.8
Pymatgen version
The latest
Operating system version
No response
Current behavior
The index of the SFAC block (second column) is not compatible with ARISS.
Currently, ResWriter gives a file as follows:
TITL mp-841-R2SCAN 0.00 63.1079 -46.08987 0.000000 0.000000 (P6_3/mmc) n - 1
CELL 1.00000 3.10263 3.10263 7.56993 90.00000 90.00000 119.99999
LATT -1
SFAC O Li
Li 1 0.00000000 -0.00000000 -0.00000000 1.000000 -0.00
Li 2 0.00000000 -0.00000000 0.50000000 1.000000 -0.00
Li 3 0.33333300 0.66666700 0.25000000 1.000000 0.00
Li 4 0.66666700 0.33333300 0.75000000 1.000000 0.00
O 5 0.33333300 0.66666700 0.64856887 1.000000 0.00
O 6 0.66666700 0.33333300 0.14856887 1.000000 0.00
O 7 0.66666700 0.33333300 0.35143113 1.000000 0.00
O 8 0.33333300 0.66666700 0.85143113 1.000000 0.00
END
Expected Behavior
The index must match the index/order of the elements in the SFAC line, not the atomic index, as shown below.
TITL cabal-in-out 0.000000000000 95.523851950751 0.000000000000 0.00 0.00 15 (n/a) n - 1
CELL 1.54180 4.48006 4.48006 5.49558 90.00000 90.00000 120.00000
LATT -1
SFAC Li Ni O
Li 1 -0.1813932000000 -0.4270413000000 0.4149065000000 1.0
Li 1 0.4270413000000 0.2456481000000 0.4149065000000 1.0
Li 1 -0.2456481000000 0.1813932000000 0.4149065000000 1.0
Li 1 -0.3333333000000 0.3333333000000 -0.4486238000000 1.0
Li 1 0.0000000000000 0.0000000000000 0.3052228000000 1.0
Ni 2 0.3294838000000 -0.1925651000000 0.2087146000000 1.0
Ni 2 0.1925651000000 -0.4779511000000 0.2087146000000 1.0
Ni 2 0.4779511000000 -0.3294838000000 0.2087146000000 1.0
Ni 2 -0.3333333000000 0.3333333000000 0.0703588000000 1.0
Ni 2 -0.0000000000000 -0.0000000000000 -0.1219930000000 1.0
O 3 -0.3333334000000 -0.0977824000000 -0.4510226000000 1.0
O 3 0.0977824000000 -0.2355510000000 -0.4510226000000 1.0
O 3 0.2355510000000 0.3333334000000 -0.4510226000000 1.0
O 3 0.3333333000000 -0.3333333000000 -0.1021862000000 1.0
O 3 -0.3333333000000 0.3333333000000 -0.2065545000000 1.0
END
Minimal example
No response
Relevant files to reproduce this bug
No response
I never use AIRSS, so just provide some info here, waiting a true master being called :).
I assume update: https://github.com/materialsproject/pymatgen/blob/668fa574ab1e3e102065c5a9d7a785213dc5ac2f/pymatgen/io/res.py#L257-L269
with this might work:
@classmethod
def _ions_from_sites(cls, sites: list[PeriodicSite]) -> list[Ion]:
"""Produce a list of entries for a SFAC block from a list of pymatgen PeriodicSite."""
ions: list[Ion] = []
i = 0
for site in sites:
+ prev_specie = None
for specie, occ in site.species.items():
- i += 1
+ if specie != prev_specie:
+ i += 1
+ prev_specie = specie
x, y, z = map(float, site.frac_coords)
spin = site.properties.get("magmom")
spin = spin and float(spin)
ions.append(Ion(specie, i, (x, y, z), occ, spin))
return ions
But I'm afraid I'm not the right person to make this change as I cannot write a proper unit test for this.
Supporting Info
From the official documentation here, there is:
$ cat Al-72120-6057-19.res
TITL Al-72120-6057-19 0.0000000000 8000.0000000000 -44.3248941562 0 0 13 (Ih) n - 1
REM
REM in /Users/user/examples/1.2
REM
REM
REM
REM
REM
REM buildcell < ./Al.cell (8f0fc5c9d882ed20c27978dd052da9d4)
REM AIRSS Version 0.9.1 July 2018 build 92bfd83db9d4+ Sat, 30 Jun 2018 13:13:37 +0100
REM compiler GCC version 5.5.0
REM options -fPIC -feliminate-unused-debug-symbols -mmacosx-version-min=10.13.6 -mtune=core2 -g -O0
REM seed -1471360510 667860809 1027640838 1038292373 -213802532 -1539206485 -1872642957 -340800072 -697171857 -761177086 -1542735574 -885338462
REM
CELL 1.54180 20.00000 20.00000 20.00000 90.00000 90.00000 90.00000
LATT -1
SFAC Al
Al 1 0.6038930453823 0.4836194826238 0.5253309492534 1.0
Al 1 0.5723861716435 0.5636803790552 0.4509205384933 1.0
Al 1 0.4999999997846 0.5000000000790 0.5000000075654 1.0
Al 1 0.4276138527499 0.4363195677348 0.5490794435604 1.0
Al 1 0.4701141676138 0.6024370215993 0.4821888755138 1.0
Al 1 0.5298858276893 0.3975629918030 0.5178112246213 1.0
Al 1 0.4526399878259 0.4244380397184 0.4387532754310 1.0
Al 1 0.5473600525560 0.5755619778738 0.5612466872006 1.0
Al 1 0.4789066108987 0.5271042987692 0.3974126364780 1.0
Al 1 0.3961069520357 0.5163804721092 0.4746690478913 1.0
Al 1 0.5210933793772 0.4728958126855 0.6025874103552 1.0
Al 1 0.4384134677060 0.5463289943787 0.5759240890051 1.0
Al 1 0.5615864847371 0.4536709615702 0.4240759146313 1.0
END
And the source code cabal.f90:
subroutine write_res()
integer :: n
character(len=40) :: fmt,ctemp,ctemp2
if((buff_words(1,1).eq.'TITL').and.(buff_words(10,1).eq.'n')) then
write(stdout,'(a)') trim(adjustl(buff(1)))
else
write(ctemp,'(f15.6)') lattice_volume
write(ctemp2,*) num_ions
write (stdout,'(a,a,a,a,f4.1,a,f4.1,2i2,1x,a,a)') 'TITL cabal-',trim(inext),'2',&
trim(outext),0.0_dp,' '//trim(adjustl(ctemp)),0.0_dp,0,0,trim(adjustl(ctemp2)),' (n/a) n - 1'
end if
!!$ write (stdout,'(a)') 'REM'
!!$ write (stdout,'(a,a)') 'REM Converted by cabal ',date()
!!$ write (stdout,'(a)') 'REM'
write (stdout,'(a,6f11.5)') 'CELL 1.54180',lattice_abc
write (stdout,'(a)') 'LATT -1'
write (ctemp,*) num_species
write (fmt,*) "(a,"//trim(adjustl(ctemp))//"a3)"
write (stdout,trim(adjustl(fmt))) 'SFAC ',species_names(1:num_species)
if(any(ion_spin.ne.0.0_dp)) then
do n=1,num_ions
write (stdout,'(a4,i4,3f17.13,f4.1,f7.2)') &
species_names(ion_index(n)),ion_index(n),ion_fractional(:,n),ion_occ(n),ion_spin(n)
end do
else
do n=1,num_ions
write (stdout,'(a4,i4,3f17.13,f4.1)') &
species_names(ion_index(n)),ion_index(n),ion_fractional(:,n),ion_occ(n)
end do
end if
write (stdout,'(a)') 'END'
end subroutine write_res
There seems to be indeed a bug as the ion_index(n) should be written not the num_ions.
Hi thanks for pinging me. But I'm not a maintainer, nor a AIRSS user. Maybe you should ping @janosh (he is recently very busy)?
Meanwhile fixing the issue itself seems easy, we need to update the unit test on which unfortunately I cannot help much...
thanks for bringing this up @SehunJoo and for suggesting a fix @DanielYang59! 👍
i'm not familiar with the RES spec either but based on the example @DanielYang59 shared, looks like this is indeed an issue.
@stefsmeets if you have time, could you take a look and comment whether this needs fixing?
This is indeed a bug, see shelx documentation here: https://shelx.uni-goettingen.de/shelxl_html.php
Seems easy enough to fix, I'll have a look.
