Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Behavior of standardize_cell(cell).atoms #150

Closed
jywu20 opened this issue Oct 10, 2023 · 3 comments · Fixed by #152
Closed

Behavior of standardize_cell(cell).atoms #150

jywu20 opened this issue Oct 10, 2023 · 3 comments · Fixed by #152
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@jywu20
Copy link

jywu20 commented Oct 10, 2023

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
Copy link
Owner

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 singularitti added the good first issue Good for newcomers label Oct 11, 2023
@singularitti
Copy link
Owner

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

@singularitti
Copy link
Owner

singularitti commented Oct 11, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants