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

Should Atoms.get_chemical_indices() return pointer? #341

Closed
samwaseda opened this issue Sep 6, 2021 · 2 comments · Fixed by #350
Closed

Should Atoms.get_chemical_indices() return pointer? #341

samwaseda opened this issue Sep 6, 2021 · 2 comments · Fixed by #350
Labels
bug Something isn't working question Further information is requested

Comments

@samwaseda
Copy link
Member

I find the following lines a bit counterintuitive.

>>> structure = pr.create.structure.bulk('Al', cubic=True)
>>> structure[0] = 'Fe'
>>> indices = structure.get_chemical_indices()
>>> print(indices)
>>> structure[:] = 'Al'
>>> print(indices)
[1 0 0 0]
[0 0 0 0]

I would rather expect that Atoms.get_chemical_indices() returns a copy of the indices and not the pointer, also because changing indices has (for some reason) no effect on the atom species. But before I start changing this I just wanted to make sure that this is not the desired behaviour. @sudarsan-surendralal @pmrv @jan-janssen

@samwaseda samwaseda added the question Further information is requested label Sep 6, 2021
@sudarsan-surendralal sudarsan-surendralal added the bug Something isn't working label Sep 6, 2021
@sudarsan-surendralal
Copy link
Member

I agree, this should not return a pointer. Go ahead with the changes!

@liamhuber
Copy link
Member

Also agreed. It's sometimes nice to have a pointer, but I would say structure.indices -> pointer; while structure.get_chemical_indices() -> new array as you suggest here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants