singularitti / Spglib.jl

A Julia wrapper for the spglib C-API
https://singularitti.github.io/Spglib.jl/
MIT License
18 stars 10 forks source link

Behavior of `standardize_cell(cell).atoms` #150

Closed jywu20 closed 1 year ago

jywu20 commented 1 year ago

Describe the bug I don't know whether it's a bug or a feature, but standardize_cell erases off the atomic number information of the cell passed to it: thus suppose we have

julia> TaSe2_1T_cell.atoms
3-element Vector{Int64}:
 73
 34
 34

and we get

julia> refine_cell(TaSe2_1T_cell, 1e-3).atoms
3-element Vector{Int32}:
 1
 2
 2

To Reproduce Steps to reproduce the behavior:

  1. Start Julia REPL
  2. Construct whatever a Cell
  3. Run standardize_cell(cell).atoms, and you will find it's an array consisting of numbers 1, 2, 3, etc. which distinguish different atoms but tell us nothing about what these atoms actually are.

Expected behavior

standardize_cell(cell).atoms == cell.atoms

Additional context The cause of the behavior is that at the end of standardize_cell(cell::AbstractCell, symprec=1e-5; to_primitive=false, no_idealize=false), the atoms field of the return value is set to atoms[1:num_atom_std]. The definition of atoms is

lattice, _positions, _atoms = _expand_cell(cell)
# ...
atoms = Vector{Cint}(undef, num_atom * allocations)
# ...
atoms[1:num_atom] .= _atoms

And if we go to the function _expand_cell, we find the following lines:

atoms = collect(Cint, findfirst(isequal(u), unique(atoms)) for u in atoms)
# ...
return lattice, positions, atoms, magmoms

So indeed _atoms (hence standardize_cell(cell).atoms) in standardize_cell contains the aforementioned indices 1, 2, 3, etc. which distinguish different atoms but tell us nothing about what these atoms actually are.

I don't know if there is any reason to not set standardize_cell(cell).atoms to cell.atoms aftrer everything is finished.

singularitti commented 1 year ago

Hi @jywu20, this is a known behavior (See this comment). The Python code did remove extra information contained in atoms, but I think it is better we recover this information. Please see if #152 solves your problem. I have added some tests so I suppose it should work.

singularitti commented 1 year ago

The CI did not work because I introduced some breaking changes into the main branch. Please use the 150 branch directly and test.

singularitti commented 1 year ago

I guess I will just release a patch version for you to test. That's easier. I have tested it on my machine so it should work.